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

[Jason Wang <jasowang@redhat.com>]

å 2022/11/25 18:37, Cornelia Huck åé:
> On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:
>
> > å 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?
> I'm not sure that adding a feature-specific status bit would make sense,
> given that the status bits either need to work before feature
> negotiation is complete, or are actually needed for feature negotiation.


It can work only after the feature negotiation. E.g the device stop 
(only makes sense after DRIVER_OK is set).


> Also, the status-bit space is way more limited than the feature-bit
> space. Therefore, I think we can safely ignore the status bits.
>
> > > 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?
> I'd say it's implied in the feature bit handling already.


Ok.

Thanks

>