Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Jason Wang <jasowang@redhat.com>]

å 2022/11/24 20:05, Cornelia Huck åé:
> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
> > > > > On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > Feature negotiation forms the basis of forward compatibility
> > > > > > guarantees of virtio but has never been properly documented.
> > > > > > Do it now.
> > > > > >
> > > > > > Suggested-by: Halil Pasic <pasic@linux.ibm.com>
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >   content.tex | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > > > >   1 file changed, 42 insertions(+)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex
> > > > > > index 3051399..e3203be 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
> > > > > >   In particular, new fields in the device configuration space are
> > > > > >   indicated by offering a new feature bit.
> > > > > >
> > > > > > +To keep te feature negotiation mechanism extensible, it is important
> > > > > > +that devices \em{do not} offer any feature bits that they would not be
> > > > > > +able to handle if the driver accepted them (even though drivers are not
> > > > > > +supposed to accept them in the first place even if offered, according to
> > > > > > +this version of the specification.)
> > > > > It looks to me if we want to clarify like this, feature negotiation is
> > > > > not sufficient. Do we need to do something similar in other basic
> > > > > facilities? Generally, we probably need to do this for facilities that
> > > > > are similar to features (status, virtqueue size and others).
> > > > I'm not sure about "not sufficient". It's sufficient as long
> > > > as you just want to extend features. What triggered this
> > > > work is adding a transport specific feature.
> > > E.g:
> > >
> > > For status: Devices do not offer any status bit it would not be able to handle.
> > > For virtqueue size:  Devices do not offer virtqueue size it would not
> > > be able to handle.
> > >
> > > ?
> > Jason I think what you miss here is this part:
> >
> > "even though drivers are not
> > supposed to accept them in the first place even if offered, according to
> > this version of the specification"
> >
> > does not apply to status and virtqueue size.
> >
> >
> > Let me clarify what all this means.
> > It seems safe for a device to offer a reserved feature bit


This depends really on the behaviour of the drivers.


> > since drivers are not supposed to accept it.


So this is the case of the ADMIN_VQ.


> > This text says device must not rely on this.
> >
> > How would this apply to status or vq size? I don't see.
> Me neither... for the status, it's about either the driver noting its
> progress, or the device indicating that a reset is needed. The only case
> where setting something requires kind of an ack is FEATURES_OK, and
> there we already spell out the conditions clearly.


I basically meant something like:

Assuming we have a feature like VIRTIO_RING_F_NEW and a new status bit 
was mapped to this feature, VIRTIO_CONFIG_S_NEW. And for some reason 
this feature is reserved for some transports. Should we mention device 
does not offer VIRTIO_CONFIG_S_NEW as well, or we assume it is implied 
that we don't offer VIRTIO_CONFIG_S_NEW in this case?


> For the queue size,
> we specify that the device states what it can support, and that the
> driver may only reduce it, that seems clear enough to me.


Similar to the above, assuming a feature VIRTIO_R_F_MAXSIZE_XXX, and it 
is reserved. Should we mention that the new max virtqueue size should 
not be advertised or it is implied in the feature advertisement?

Thanks