On Fri, Oct 06, 2023 at 03:55:56PM +0200, Hanna Czenczek wrote: > On 06.10.23 10:49, Michael S. Tsirkin wrote: > > On Fri, Oct 06, 2023 at 09:53:53AM +0200, Hanna Czenczek wrote: > > > On 05.10.23 19:38, Stefan Hajnoczi wrote: > > > > On Wed, Oct 04, 2023 at 02:58:58PM +0200, Hanna Czenczek wrote: > > > > > GET_VRING_BASE does not mention that it stops the respective ring. > > > > > Fix > > > > > that. > > > > > > > > > > Furthermore, it is not fully clear what the "base offset" these > > > > > commands' documentation refers to is; an offset could be many things. > > > > > Be more precise and verbose about it, especially given that these > > > > > commands use different payload structures depending on whether the > > > > > vring > > > > > is split or packed. > > > > > > > > > > Signed-off-by: Hanna Czenczek <hre...@redhat.com> > > > > > --- > > > > > docs/interop/vhost-user.rst | 66 > > > > > ++++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 62 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst > > > > > index 2f68e67a1a..50f5acebe5 100644 > > > > > --- a/docs/interop/vhost-user.rst > > > > > +++ b/docs/interop/vhost-user.rst > > > > > @@ -108,6 +108,37 @@ A vring state description > > > > > :num: a 32-bit number > > > > > +A vring descriptor index for split virtqueues > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > + > > > > > ++-------------+---------------------+ > > > > > +| vring index | index in avail ring | > > > > > ++-------------+---------------------+ > > > > > + > > > > > +:vring index: 32-bit index of the respective virtqueue > > > > > + > > > > > +:index in avail ring: 32-bit value, of which currently only the > > > > > lower 16 > > > > > + bits are used: > > > > > + > > > > > + - Bits 0–15: Next descriptor index in the *Available Ring* > > > > I think we need to say more to make this implementable just by reading > > > > the spec: > > > > > > > > Index of the next *Available Ring* descriptor that the back-end will > > > > process. This is a free-running index that is not wrapped by the > > > > ring > > > > size. > > > Sure, thanks. > > > > > > > Feel free to rephrase. > > > > > > > > > + - Bits 16–31: Reserved (set to zero) > > > > > + > > > > > +Vring descriptor indices for packed virtqueues > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > + > > > > > ++-------------+--------------------+ > > > > > +| vring index | descriptor indices | > > > > > ++-------------+--------------------+ > > > > > + > > > > > +:vring index: 32-bit index of the respective virtqueue > > > > > + > > > > > +:descriptor indices: 32-bit value: > > > > > + > > > > > + - Bits 0–14: Index in the *Available Ring* > > > > Same here. > > > > > > > > > + - Bit 15: Driver (Available) Ring Wrap Counter > > > > > + - Bits 16–30: Index in the *Used Ring* > > > > Same here. > > > > > > > > > + - Bit 31: Device (Used) Ring Wrap Counter > > > > > + > > > > > A vring address description > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > @@ -1031,18 +1062,45 @@ Front-end message types > > > > > ``VHOST_USER_SET_VRING_BASE`` > > > > > :id: 10 > > > > > :equivalent ioctl: ``VHOST_SET_VRING_BASE`` > > > > > - :request payload: vring state description > > > > > + :request payload: vring descriptor index/indices > > > > > :reply payload: N/A > > > > > - Sets the base offset in the available vring. > > > > > + Sets the next index to use for descriptors in this vring: > > > > > + > > > > > + * For a split virtqueue, sets only the next descriptor index in the > > > > > + *Available Ring*. The device is supposed to read the next index > > > > > in > > > > > + the *Used Ring* from the respective vring structure in guest > > > > > memory. > > > > > + > > > > > + * For a packed virtqueue, both indices are supplied, as they are > > > > > not > > > > > + explicitly available in memory. > > > > > + > > > > > + Consequently, the payload type is specific to the type of virt > > > > > queue > > > > > + (*a vring descriptor index for split virtqueues* vs. *vring > > > > > descriptor > > > > > + indices for packed virtqueues*). > > > > > ``VHOST_USER_GET_VRING_BASE`` > > > > > :id: 11 > > > > > :equivalent ioctl: ``VHOST_USER_GET_VRING_BASE`` > > > > > :request payload: vring state description > > > > > - :reply payload: vring state description > > > > > + :reply payload: vring descriptor index/indices > > > > > + > > > > > + Stops the vring and returns the current descriptor index or > > > > > indices: > > > > > + > > > > > + * For a split virtqueue, returns only the 16-bit next descriptor > > > > > + index in the *Available Ring*. The index in the *Used Ring* is > > > > > + controlled by the guest driver and can be read from the vring > > > > I find "is controlled by the guest driver" confusing. The device writes > > > > the Used Ring index. The driver only reads it. The device is the active > > > > party here. > > > Er, good point. That breaks the whole reasoning. Then I don’t understand > > > why we do get/set the available ring index and not the used ring index. > > > Do > > > you know why? > > It's simple. used ring index in memory is controlled by the device and > > reflects device state. > > Exactly, it’s device state, that’s why I thought the front-end needs to > ensure its read and restored around the reset we currently have in > vhost_dev_stop()/start(). > > > device can just read it back to restore. > > I find it strange that the device is supposed to read its own state from > memory.
/me shrugs. It puts it there, why not read it back. Duplicating state is not usually a good idea - leads to bugs. > > available ring index in memory is controlled by driver and does > > not reflect device state. > > Why can’t the device read the available index from memory? That value is > put into memory by the driver precisely so the device can read it from > there. > > Hanna Consider an example of RX ring for net device. buffers might be available but device does not use them until packets arrive. what I think you could say is that actually just the used index should be sufficient. So I think main thing GET_BASE does is stop the ring. As for the value returned, we can if we want to validate that it matches used ring index. -- MST
