Re: [virtio-comment] [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Mar 25, 2021 at 10:42:38AM +0800, Jason Wang wrote:
> 
> å 2021/3/24 äå5:38, Stefan Hajnoczi åé:
> > On Wed, Mar 24, 2021 at 04:15:55PM +0800, Jason Wang wrote:
> > > å 2021/3/23 äå6:27, Stefan Hajnoczi åé:
> > > > On Mon, Mar 22, 2021 at 11:47:16AM +0800, Jason Wang wrote:
> > > > > +The \field{used_wrap_counter} field is the wrap counter that is used
> > > > > +by the device.
> > > > > +
> > > > > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not
> > > > > +negotiated.
> > > > > +
> > > > > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> > > > > +
> > > > > +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> > > > > +
> > > > > +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver MUST set the
> > > > > +state of each virtqueue after setting the FEATURE_OK status bit but
> > > > > +before setting the DRIVER_OK status bit.
> > > > > +
> > > > > +If VIRTIO_F_QUEUE_STATE has been negotiated. a driver MUST reset the
> > > > s/negotiated./negotiated,/
> > > > 
> > > > > +device before getting the virtqueue state.
> > > > Interesting approach, but it makes sense that the device must be stopped
> > > > before we mess with the queue state :).
> > > > 
> > > > I wonder whether requiring the device to keep state across reset will
> > > > cause issues in the future or for testing/fuzzing.
> > > > 
> > > > Another approach would have been to add a new status register bit for
> > > > pausing the device. That reminds me of vhost.
> > > 
> > > That's the way I did here:
> > > https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html
> > That design seems less risky. I think modifying the semantics of device
> > reset might end up being a problem. Defining an explicit paused state
> > for saving/loading state seems cleaner.
> > 
> > Why did you end up switching approaches?
> 
> 
> I try to seek some short-cut withut introducing new status bit but I end up
> realizing it might not work as expected.
> 
> Another reason is that, by using new status bit, we need to ability to
> resume the device by clearing the bit which is somehow conflict with what
> spec requries:
> 
> "
> 
> 2.1.1 Driver Requirements: Device Status Field
> 
> The driver MUST NOT clear adevice statusbit.
> 
> "
> 
> Maybe we can have an exception for the device stop/pause in this case.

Yes, I think that is simply because the VIRTIO device lifecycle never
needed to go back to a previous lifecycle state. It was a unidirectional
state machine.

The device stop/pause state seems like a valid case of going back to a
previous lifecycle state and I don't think it breaks anything in the
device model.

Stefan

Attachment: signature.asc
Description: PGP signature