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

[Jason Wang <jasowang@redhat.com>]

å 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 aÂdevice statusÂbit.

"

Maybe we can have an exception for the device stop/pause in this case.

Thanks

> Stefan
>