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

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 11/7/2023 4:22 PM, Michael S. Tsirkin wrote:
> 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.
Thanks for your advice, I may need more hints to improve this patch.

If it is about when _F_IN_ORDER not negotiated, I found this section 
from the spec:

Some devices always use descriptors in the same order in which they have 
been made available. These
devices can offer the VIRTIO_F_IN_ORDER feature. If negotiated, this 
knowledge allows devices to notify
the use of a batch of buffers to the driver by only writing out a single 
used ring entry with the id corresponding
to the head entry of the descriptor chain describing the last buffer in 
the batch.
The device then skips forward in the ring according to the size of the 
batch. Accordingly, it increments the
used idx by the size of the batch.

This section implies that if _F_IN_ORDER is not negotiated, the device 
may not able to process the descriptors in
order, thus may not write only one used_idx for a batch of buffers. This 
is about how to make buffer used and
used_idx is in the guest memory. If the device selective done processing 
some descriptors, then maybe just
mark them done one by one than batching.

Here I see there are two kind of vq states, on device or in guest 
memory. So this series migrate the on device
state explicitly and others are migrating with guest memory.

Can you be more specific on what parameters of a vq that I should 
address in this patch?

Thanks
>