Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero

[Cornelia Huck]

On Thu, 25 Feb 2021 12:36:07 +0800
Jason Wang <jasowang@xxxxxxxxxx> wrote:

> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >  
> >> On 2021/2/23 6:58 下午, Cornelia Huck wrote:  
> >>> On Tue, 23 Feb 2021 18:31:07 +0800
> >>> Jason Wang <jasowang@xxxxxxxxxx> wrote:

> >>>> The problem is the MTU description for example:
> >>>>
> >>>> "The following driver-read-only field, mtu only exists if
> >>>> VIRTIO_NET_F_MTU is set."
> >>>>
> >>>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".  
> >>> "offered by the device"? I don't think it should 'disappear' from the
> >>> config space if the driver won't use it. (Same for other config space
> >>> fields that are tied to feature bits.)  
> >>
> >> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> >> to according to the spec there will be no mtu field.  
> > I think so, yes.
> >  
> >> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> >> VIRTIO_NET_F_MTU offered. To me, it means we don't have
> >> max_virtqueue_pairs but it's not how the driver is wrote today.  
> > That would be a bug, but it seems to me that the virtio-net driver
> > reads max_virtqueue_pairs conditionally and handles absence of the
> > feature correctly?  
> 
> 
> Yes, see the avove codes:
> 
>          if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
>                  int mtu = virtio_cread16(vdev,
>                                           offsetof(struct virtio_net_config,
>                                                    mtu));
>                  if (mtu < MIN_MTU)
>                          __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
>          }
> 

Ouch, you're right. The virtio_cread accessors all operate on offsets
into a structure, it's just more obvious here.

> So it's probably too late to fix the driver.

It is never too late to fix a driver :)

It seems involved, though. We'd need different config space structures
based upon which set of features has been negotiated. It's not too bad
when features build upon each other and add fields at the end (this
should be fine right now, if the code remembered to check for the
feature), but can become ugly if an in-between field depends upon a
feature.

I guess we've been lucky that it seems to be an extremely uncommon
configuration.

> 
> 
> >  
> >>  
> >>>        
> >>>> Otherwise readers (at least for me), may think the MTU is only valid
> >>>> if driver set the bit.  
> >>> I think it would still be 'valid' in the sense that it exists and has
> >>> some value in there filled in by the device, but a driver reading it
> >>> without negotiating the feature would be buggy. (Like in the kernel
> >>> code above; the kernel not liking the value does not make the field
> >>> invalid.)  
> >>
> >> See Michael's reply, the spec allows read the config before setting
> >> features.  
> > Yes, the period prior to finishing negotiation is obviously special.
> >  
> >>  
> >>> Maybe a statement covering everything would be:
> >>>
> >>> "The following driver-read-only field mtu only exists if the device
> >>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> >>> negotiation and after VIRTIO_NET_F_MTU has been successfully
> >>> negotiated."
> >>>     
> >>>>     
> >>>>> Should we add a wording clarification to the spec?  
> >>>> I think so.  
> >>> Some clarification would be needed for each field that depends on a
> >>> feature; that would be quite verbose. Maybe we can get away with a
> >>> clarifying statement?
> >>>
> >>> "Some config space fields may depend on a certain feature. In that
> >>> case, the field exits if the device has offered the corresponding
> >>> feature,  
> >>
> >> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> >> will look like:
> >>
> >> struct virtio_net_config {
> >>           u8 mac[6];
> >>           le16 status;
> >>           le16 mtu;
> >> };
> >>  
> > I agree.  
> 
> 
> So consider it's probably too late to fix the driver which assumes some 
> field are always persent, it looks to me need fix the spec do declare 
> the fields are always existing instead.

The problem with that is that it has been in the spec already, and a
compliant device that did not provide the fields without the features
would suddenly become non-compliant...

> 
> 
> >  
> >>>    and may be read by the driver during feature negotiation, and
> >>> accessed by the driver after the feature has been successfully
> >>> negotiated. A shorthand for this is a statement that a field only
> >>> exists if a certain feature bit is set."  
> >>
> >> I'm not sure using "shorthand" is good for the spec, at least we can
> >> limit the its scope only to the configuration space part.  
> > Maybe "a shorthand expression"?  
> 
> 
> So the questions is should we use this for all over the spec or it will 
> be only used in this speicifc part (device configuration).

For command structures and the like, "feature is set" should always
mean "feature has been negotiated"; I think config space is special
because the driver can read it before feature negotiation is finished,
so device configuration is probably the proper place for it.