thr3ads.net - Virtualization - [PATCH] vdpa/mlx5: set_features should allow reset to zero [Feb 2021]

If this information is useful, please help other people find it:
Share via:

Si-Wei Liu

2021-Feb-22 17:09 UTC

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

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

-Siwei
>
> Thanks
>
>
>> Rejecting reset to 0
>> prematurely causes correct MTU and link status unable to load
>> for the very first config space access, rendering issues like
>> guest showing inaccurate MTU value, or failure to reject
>> out-of-range MTU.
>>
>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported
mlx5
>> devices")
>> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
>> ---
>> ? drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +--------------
>> ? 1 file changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 7c1f789..540dd67 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1490,14 +1490,6 @@ static u64 mlx5_vdpa_get_features(struct 
>> vdpa_device *vdev)
>> ????? return mvdev->mlx_features;
>> ? }
>> ? -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 
>> features)
>> -{
>> -??? if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>> -??????? return -EOPNOTSUPP;
>> -
>> -??? return 0;
>> -}
>> -
>> ? static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
>> ? {
>> ????? int err;
>> @@ -1558,18 +1550,13 @@ static int mlx5_vdpa_set_features(struct 
>> vdpa_device *vdev, u64 features)
>> ? {
>> ????? struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>> ????? struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>> -??? int err;
>> ? ????? print_features(mvdev, features, true);
>> ? -??? err = verify_min_features(mvdev, features);
>> -??? if (err)
>> -??????? return err;
>> -
>> ????? ndev->mvdev.actual_features = features &
ndev->mvdev.mlx_features;
>> ????? ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, ndev->mtu);
>> ????? ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>> VIRTIO_NET_S_LINK_UP);
>> -??? return err;
>> +??? return 0;
>> ? }
>> ? ? static void mlx5_vdpa_set_config_cb(struct vdpa_device *vdev, 
>> struct vdpa_callback *cb)
>

Jason Wang

2021-Feb-23 02:03 UTC

head link

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

On 2021/2/23 1:09 ??, 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?

Yes, but the problem is we can't detect whether or not it's an legacy 
guest (e.g feature is not set).

>
> I think the point is, since there's legacy guest we'd have to
support,
> this host side workaround is unavoidable.

Since from vhost-vDPA point of view the driver is Qemu, it means we need 
make qemu vhost-vDPA driver spec complaint.

E.g how about:

1) revert 452639a64ad8 and fix Qemu? In Qemu, during vhost-vDPA 
initialization, do a minial config sync by neogitating minimal features 
(e.g just VIRTIO_F_ACCESS_PLATFORM). When FEATURE_OK is not set from 
guest, we can only allow it to access the config space that is emulated 
by Qemu?

Then

> Although I agree the violating driver should be fixed (yes, it's in 
> today's upstream kernel which exists for a while now).

2) Fix the virtio driver bug.

Or a quick workaround is to set (VIRTIO_F_ACCESS_PLATFORM instead of 0) 
in this case.

Thanks

>
> -Siwei
>
>>
>> Thanks
>>
>>
>>> Rejecting reset to 0
>>> prematurely causes correct MTU and link status unable to load
>>> for the very first config space access, rendering issues like
>>> guest showing inaccurate MTU value, or failure to reject
>>> out-of-range MTU.
>>>
>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported
mlx5
>>> devices")
>>> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
>>> ---
>>> ? drivers/vdpa/mlx5/net/mlx5_vnet.c | 15 +--------------
>>> ? 1 file changed, 1 insertion(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 7c1f789..540dd67 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1490,14 +1490,6 @@ static u64 mlx5_vdpa_get_features(struct 
>>> vdpa_device *vdev)
>>> ????? return mvdev->mlx_features;
>>> ? }
>>> ? -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 
>>> features)
>>> -{
>>> -??? if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>> -??????? return -EOPNOTSUPP;
>>> -
>>> -??? return 0;
>>> -}
>>> -
>>> ? static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
>>> ? {
>>> ????? int err;
>>> @@ -1558,18 +1550,13 @@ static int mlx5_vdpa_set_features(struct 
>>> vdpa_device *vdev, u64 features)
>>> ? {
>>> ????? struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> ????? struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -??? int err;
>>> ? ????? print_features(mvdev, features, true);
>>> ? -??? err = verify_min_features(mvdev, features);
>>> -??? if (err)
>>> -??????? return err;
>>> -
>>> ????? ndev->mvdev.actual_features = features & 
>>> ndev->mvdev.mlx_features;
>>> ????? ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, ndev->mtu);
>>> ????? ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>>> VIRTIO_NET_S_LINK_UP);
>>> -??? return err;
>>> +??? return 0;
>>> ? }
>>> ? ? static void mlx5_vdpa_set_config_cb(struct vdpa_device *vdev, 
>>> struct vdpa_callback *cb)
>>
>

Michael S. Tsirkin

2021-Feb-23 09:25 UTC

head link

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

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);
        }

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

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.

-- 
MST

Virtualization - Feb 2021 - [PATCH] vdpa/mlx5: set_features should allow reset to zero

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

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

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