Re: [PATCH V2 1/6] virtio: introduce virtqueue state

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 07, 2023 at 04:11:25PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 11/6/2023 5:45 PM, Michael S. Tsirkin wrote:
> > On Mon, Nov 06, 2023 at 05:42:10PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 11/6/2023 5:35 PM, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 03, 2023 at 10:49:42PM +0800, Zhu, Lingshan wrote:
> > > > >           +When SUSPEND is set, the device MUST record the Available State of every enabled splited virtqueue
> > > > >           +in \field{Available State} field,
> > > > >           +and correspondingly restore the Available State of every enabled splited virtqueue
> > > > >           +from \field{Available State} field when DRIVER_OK is set.
> > > > >           +
> > > > >           +The device SHOULD reset \field{Available State} field upon a device reset.
> > > > > 
> > > > >       At this point I have no idea
> > > > >       - how can a state of a virtqueue at a random time be represented
> > > > >         by a 16 bit integer
> > > > > 
> > > > > not sure what is a random time, this is to request the device to reset
> > > > > its avail state, for example, it is "le16 queue_avail_state" in virtio-pci
> > > > > common cfg. Resetting this so the device will not recover from a wrong value of
> > > > > the last run.
> > > > You simply never bother to say what is "Available State" and what
> > > > does it mean to restore it.  Not to mention words like "splited"
> > > > which just adds to the confusion.
> > > It says:
> > > +The available state field is two bytes of virtqueue state that is used by
> > > +the device to read the next available buffer. It is presented in the
> > > following format:
> > > 
> > > Do you want me to add more descriptions?
> > maybe start with an example
> I think they are already in the spec, I can add:
> see also "2.7.6 The Virtqueue Available Ring" and "2.7.13.1 Placing Buffers
> Into The Descriptor Table"
> > 
> > > > >       - if it's not at a random time then why do you even need an integer -
> > > > >         synchronize queue to memory and then all state is in memory
> > > > > 
> > > > > Not sure what is a sync queue, but for example, "le16 queue_avail_state" for
> > > > > PCI transport exists in a cap.
> > > > I just point out that normally a lot of ring state is in memory.
> > > > So you need to be much more specific about how you are augmenting that.
> > > > For example, if buffers are used exactly in order for a split ring
> > > > then used index seems to be exactly the same as last available index
> > > > you describe - it's a free running counter. OTOH if they are not
> > > > used in order then I don't see how is a single index sufficient to
> > > > describe which ones have been used and which not.
> > > I am not sure I get it.
> > > 
> > > Used idx(not like packed vq, no over-writing descriptors) and other states
> > > are in guest memory, so migrated with guest migration.
> > yes and so? why is that not enough and what is this available state then?
> The spec has illustrated how available index work and has given an
> example(see above cited sections)
> And this patch even has given a more clear description for it.
> 
> Other states are in guest memory and migrated with guest memory.

Yea I wrote large parts of it and I know how the available index works.

And sorry no idea what you are talking about.

At any time, there can be up to 2^16 buffers that have been made
available, and a random subset of these have been used. There is no
chance in the world a single 16 bit index describes even that part of
state, never mind device type specific processing that might be going
on.

As a wild guest this proposal is making a bunch of unstated assumptions
about device being in a very specific state where this *is* possible.
For people to be able to implement devices and drivers these
need to be spelled out.


-- 
MST