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

[Jason Wang <jasowang@redhat.com>]

On 2021/2/23 6:17 下午, Jason Wang wrote:
>
> On 2021/2/23 6:01 下午, Michael S. Tsirkin wrote:
>> On Tue, Feb 23, 2021 at 05:46:20PM +0800, Jason Wang wrote:
>>> On 2021/2/23 下午5:25, Michael S. Tsirkin wrote:
>>>> On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wrote:
>>>>> On 2/21/2021 8:14 PM, Jason Wang wrote:
>>>>>> On 2021/2/19 7:54 下午, Si-Wei Liu wrote:
>>>>>>> Commit 452639a64ad8 ("vdpa: make sure set_features is invoked
>>>>>>> for legacy") made an exception for legacy guests to reset
>>>>>>> features to 0, when config space is accessed before features
>>>>>>> are set. We should relieve the verify_min_features() check
>>>>>>> and allow features reset to 0 for this case.
>>>>>>>
>>>>>>> It's worth noting that not just legacy guests could access
>>>>>>> config space before features are set. For instance, when
>>>>>>> feature VIRTIO_NET_F_MTU is advertised some modern driver
>>>>>>> will try to access and validate the MTU present in the config
>>>>>>> space before virtio features are set.
>>>>>> This looks like a spec violation:
>>>>>>
>>>>>> "
>>>>>>
>>>>>> The following driver-read-only field, mtu only exists if
>>>>>> VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for 
>>>>>> the
>>>>>> driver to use.
>>>>>> "
>>>>>>
>>>>>> Do we really want to workaround this?
>>>>> Isn't the commit 452639a64ad8 itself is a workaround for legacy 
>>>>> guest?
>>>>>
>>>>> I think the point is, since there's legacy guest we'd have to 
>>>>> support, this
>>>>> host side workaround is unavoidable. Although I agree the 
>>>>> violating driver
>>>>> should be fixed (yes, it's in today's upstream kernel which exists 
>>>>> for a
>>>>> while now).
>>>> Oh  you are right:
>>>>
>>>>
>>>> static int virtnet_validate(struct virtio_device *vdev)
>>>> {
>>>>           if (!vdev->config->get) {
>>>>                   dev_err(&vdev->dev, "%s failure: config access 
>>>> disabled\n",
>>>>                           __func__);
>>>>                   return -EINVAL;
>>>>           }
>>>>
>>>>           if (!virtnet_validate_features(vdev))
>>>>                   return -EINVAL;
>>>>
>>>>           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);
>>>
>>> I wonder why not simply fail here?
>> Back in 2016 it went like this:
>>
>>     On Thu, Jun 02, 2016 at 05:10:59PM -0400, Aaron Conole wrote:
>>     > +     if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
>>     > +             dev->mtu = virtio_cread16(vdev,
>>     > +                                       offsetof(struct 
>> virtio_net_config,
>>     > +                                                mtu));
>>     > +     }
>>     > +
>>     >       if (vi->any_header_sg)
>>     >               dev->needed_headroom = vi->hdr_len;
>>     >
>>
>>     One comment though: I think we should validate the mtu.
>>     If it's invalid, clear VIRTIO_NET_F_MTU and ignore.
>>
>>
>> Too late at this point :)
>>
>> I guess it's a way to tell device "I can not live with this MTU",
>> device can fail FEATURES_OK if it wants to. MIN_MTU
>> is an internal linux thing and at the time I felt it's better to
>> try to make progress.
>
>
> What if e.g the device advertise a large MTU. E.g 64K here?


Ok, consider we use add_recvbuf_small() when neither GSO nor mrg_rxbuf. 
This means we should fail the probing if MTU is greater than 1500 in 
this case.

Thanks


> In that case, the driver can not live either. Clearing MTU won't help 
> here.
>
> Thanks
>
>
>>
>>
>>>>           }
>>>>
>>>>           return 0;
>>>> }
>>>>
>>>> And the spec says:
>>>>
>>>>
>>>> The driver MUST follow this sequence to initialize a device:
>>>> 1. Reset the device.
>>>> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the 
>>>> device.
>>>> 3. Set the DRIVER status bit: the guest OS knows how to drive the 
>>>> device.
>>>> 4. Read device feature bits, and write the subset of feature bits 
>>>> understood by the OS and driver to the
>>>> device. During this step the driver MAY read (but MUST NOT write) 
>>>> the device-specific configuration
>>>> fields to check that it can support the device before accepting it.
>>>> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new 
>>>> feature bits after this step.
>>>> 6. Re-read device status to ensure the FEATURES_OK bit is still 
>>>> set: otherwise, the device does not
>>>> support our subset of features and the device is unusable.
>>>> 7. Perform device-specific setup, including discovery of virtqueues 
>>>> for the device, optional per-bus setup,
>>>> reading and possibly writing the device’s virtio configuration 
>>>> space, and population of virtqueues.
>>>> 8. Set the DRIVER_OK status bit. At this point the device is “live”.
>>>>
>>>>
>>>> Item 4 on the list explicitly allows reading config space before
>>>> FEATURES_OK.
>>>>
>>>> I conclude that VIRTIO_NET_F_MTU is set means "set in device 
>>>> features".
>>>
>>> So this probably need some clarification. "is set" is used many 
>>> times in the
>>> spec that has different implications.
>>>
>>> Thanks
>>>
>>>
>>>> Generally it is worth going over feature dependent config fields
>>>> and checking whether they should be present when device feature is set
>>>> or when feature bit has been negotiated, and making this clear.
>>>>
>