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

[Cornelia Huck <cohuck@redhat.com>]

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.
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.