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

On Thu, 25 Feb 2021 12:36:07 +0800
Jason Wang [off-list ref] wrote:

On 2021/2/24 7:12 下午, Cornelia Huck wrote:

quoted

On Wed, 24 Feb 2021 17:29:07 +0800
Jason Wang [off-list ref] wrote:
 

quoted

On 2021/2/23 6:58 下午, Cornelia Huck wrote:  

quoted

On Tue, 23 Feb 2021 18:31:07 +0800
Jason Wang [off-list ref] wrote:

quoted

quoted

quoted

quoted

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.
 

quoted

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.

quoted

quoted

quoted

quoted

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.
 

quoted

quoted

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

quoted

quoted

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

quoted

quoted

quoted

   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.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization