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

>