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

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

[Si-Wei Liu]

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

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

[Eli Cohen]

On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
> -}
> -

But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
support such cases.

Maybe we should call verify_min_features() from mlx5_vdpa_set_status()
just before attempting to call setup_driver().

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

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

[Michael S. Tsirkin]

On Sun, Feb 21, 2021 at 04:44:37PM +0200, Eli Cohen wrote:
> On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
> > -}
> > -
> 
> But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
> support such cases.

Did you mean "catch such cases" rather than "support"?


> Maybe we should call verify_min_features() from mlx5_vdpa_set_status()
> just before attempting to call setup_driver().
> 
> >  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)
> > -- 
> > 1.8.3.1
> > 

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

[Jason Wang]

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?

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

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

[Eli Cohen]

On Sun, Feb 21, 2021 at 04:52:05PM -0500, Michael S. Tsirkin wrote:
> On Sun, Feb 21, 2021 at 04:44:37PM +0200, Eli Cohen wrote:
> > On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
> > > -}
> > > -
> > 
> > But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
> > support such cases.
> 
> Did you mean "catch such cases" rather than "support"?
> 

Actually I meant this driver/device does not support such cases.

> 
> > Maybe we should call verify_min_features() from mlx5_vdpa_set_status()
> > just before attempting to call setup_driver().
> > 
> > >  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)
> > > -- 
> > > 1.8.3.1
> > > 
> 

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

[Michael S. Tsirkin]

On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> 
> Thanks

And also:

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


so accessing config space before FEATURES_OK is a spec violation, right?


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

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

[Si-Wei Liu]

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

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

[Si-Wei Liu]

On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>
>> Thanks
> And also:
>
> 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”.
>
>
> so accessing config space before FEATURES_OK is a spec violation, right?
It is, but it's not relevant to what this commit tries to address. I 
thought the legacy guest still needs to be supported.

Having said, a separate patch has to be posted to fix the guest driver 
issue where this discrepancy is introduced to virtnet_validate() (since 
commit fe36cbe067). But it's not technically related to this patch.

-Siwei

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

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

[Jason Wang]

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

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

[Jason Wang]

On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>
>
> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>
>>> Thanks
>> And also:
>>
>> 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”.
>>
>>
>> so accessing config space before FEATURES_OK is a spec violation, right?
> It is, but it's not relevant to what this commit tries to address. I 
> thought the legacy guest still needs to be supported.
>
> Having said, a separate patch has to be posted to fix the guest driver 
> issue where this discrepancy is introduced to virtnet_validate() 
> (since commit fe36cbe067). But it's not technically related to this 
> patch.
>
> -Siwei


I think it's a bug to read config space in validate, we should move it 
to virtnet_probe().

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

On Mon, Feb 22, 2021 at 08:05:26AM +0200, Eli Cohen wrote:
> On Sun, Feb 21, 2021 at 04:52:05PM -0500, Michael S. Tsirkin wrote:
> > On Sun, Feb 21, 2021 at 04:44:37PM +0200, Eli Cohen wrote:
> > > On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
> > > > -}
> > > > -
> > > 
> > > But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
> > > support such cases.
> > 
> > Did you mean "catch such cases" rather than "support"?
> > 
> 
> Actually I meant this driver/device does not support such cases.

Well the removed code merely failed without VIRTIO_F_ACCESS_PLATFORM
it didn't actually try to support anything ...

> > 
> > > Maybe we should call verify_min_features() from mlx5_vdpa_set_status()
> > > just before attempting to call setup_driver().
> > > 
> > > >  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)
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > 

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

[Jason Wang]

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?


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

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

[Jason Wang]

On 2021/2/23 下午5:26, Michael S. Tsirkin wrote:
> On Mon, Feb 22, 2021 at 08:05:26AM +0200, Eli Cohen wrote:
>> On Sun, Feb 21, 2021 at 04:52:05PM -0500, Michael S. Tsirkin wrote:
>>> On Sun, Feb 21, 2021 at 04:44:37PM +0200, Eli Cohen wrote:
>>>> On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
>>>>> -}
>>>>> -
>>>> But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
>>>> support such cases.
>>> Did you mean "catch such cases" rather than "support"?
>>>
>> Actually I meant this driver/device does not support such cases.
> Well the removed code merely failed without VIRTIO_F_ACCESS_PLATFORM
> it didn't actually try to support anything ...


I think it's used to catch the driver that doesn't support ACCESS_PLATFORM?

Thanks


>

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2021 at 05:48:10PM +0800, Jason Wang wrote:
> 
> On 2021/2/23 下午5:26, Michael S. Tsirkin wrote:
> > On Mon, Feb 22, 2021 at 08:05:26AM +0200, Eli Cohen wrote:
> > > On Sun, Feb 21, 2021 at 04:52:05PM -0500, Michael S. Tsirkin wrote:
> > > > On Sun, Feb 21, 2021 at 04:44:37PM +0200, Eli Cohen wrote:
> > > > > On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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@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;
> > > > > > -}
> > > > > > -
> > > > > But what if VIRTIO_F_ACCESS_PLATFORM is not offerred? This does not
> > > > > support such cases.
> > > > Did you mean "catch such cases" rather than "support"?
> > > > 
> > > Actually I meant this driver/device does not support such cases.
> > Well the removed code merely failed without VIRTIO_F_ACCESS_PLATFORM
> > it didn't actually try to support anything ...
> 
> 
> I think it's used to catch the driver that doesn't support ACCESS_PLATFORM?
> 
> Thanks
> 

That is why I asked whether Eli meant catch.

-- 
MST

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

[Michael S. Tsirkin]

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.


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

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

[Cornelia Huck]

On Tue, 23 Feb 2021 17:46:20 +0800
Jason Wang <jasowang@redhat.com> 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?

I think both failing or not accepting the feature can be argued to make
sense: "the device presented us with a mtu size that does not make
sense" would point to failing, "we cannot work with the mtu size that
the device presented us" would point to not negotiating the feature.

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

Before FEATURES_OK is set by the driver, I guess it means "the device
has offered the feature"; during normal usage, it means "the feature
has been negotiated". (This is a bit fuzzy for legacy mode.)

Should we add a wording clarification to the spec?

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

[Jason Wang]

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

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

[Jason Wang]

On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 17:46:20 +0800
> Jason Wang <jasowang@redhat.com> 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?
> I think both failing or not accepting the feature can be argued to make
> sense: "the device presented us with a mtu size that does not make
> sense" would point to failing, "we cannot work with the mtu size that
> the device presented us" would point to not negotiating the feature.
>
>>
>>>           }
>>>
>>>           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.
> Before FEATURES_OK is set by the driver, I guess it means "the device
> has offered the feature";


For me this part is ok since it clarify that it's the driver that set 
the bit.



> during normal usage, it means "the feature
> has been negotiated".

/?

It looks to me the feature negotiation is done only after device set 
FEATURES_OK, or FEATURES_OK could be read from device status?


>   (This is a bit fuzzy for legacy mode.)


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". 
Otherwise readers (at least for me), may think the MTU is only valid if 
driver set the bit.


>
> Should we add a wording clarification to the spec?


I think so.

Thanks

>

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

[Cornelia Huck]

On Tue, 23 Feb 2021 18:31:07 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > On Tue, 23 Feb 2021 17:46:20 +0800
> > Jason Wang <jasowang@redhat.com> 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?  
> > I think both failing or not accepting the feature can be argued to make
> > sense: "the device presented us with a mtu size that does not make
> > sense" would point to failing, "we cannot work with the mtu size that
> > the device presented us" would point to not negotiating the feature.
> >  
> >>  
> >>>           }
> >>>
> >>>           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.  
> > Before FEATURES_OK is set by the driver, I guess it means "the device
> > has offered the feature";  
> 
> 
> For me this part is ok since it clarify that it's the driver that set 
> the bit.
> 
> 
> 
> > during normal usage, it means "the feature
> > has been negotiated".  
> 
> /?
> 
> It looks to me the feature negotiation is done only after device set 
> FEATURES_OK, or FEATURES_OK could be read from device status?

I'd consider feature negotiation done when the driver reads FEATURES_OK
back from the status.

> 
> 
> >   (This is a bit fuzzy for legacy mode.)

...because legacy does not have FEATURES_OK.
  
> 
> 
> 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.)
 
> 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.)

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

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

[Michael S. Tsirkin]

On Fri, Feb 19, 2021 at 06:54:58AM -0500, 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. 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")

isn't this more

    vdpa: make sure set_features is invoked for legacy


> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>

I think we at least need to correct the comment in
include/linux/vdpa.h then

Instead of "we assume a legacy guest" we'd say something like
"call set features in case it's a legacy guest".

Generally it's unfortunate. Need to think about what to do here.
Any idea how else we can cleanly detect a legacy guest?

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

Let's just set VIRTIO_F_ACCESS_PLATFORM in core?
Then we don't need to hack mlx5 ...


> @@ -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)
> -- 
> 1.8.3.1

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> 
> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > 
> > 
> > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > 
> > > > Thanks
> > > And also:
> > > 
> > > 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”.
> > > 
> > > 
> > > so accessing config space before FEATURES_OK is a spec violation, right?
> > It is, but it's not relevant to what this commit tries to address. I
> > thought the legacy guest still needs to be supported.
> > 
> > Having said, a separate patch has to be posted to fix the guest driver
> > issue where this discrepancy is introduced to virtnet_validate() (since
> > commit fe36cbe067). But it's not technically related to this patch.
> > 
> > -Siwei
> 
> 
> I think it's a bug to read config space in validate, we should move it to
> virtnet_probe().
> 
> Thanks

I take it back, reading but not writing seems to be explicitly allowed by spec.
So our way to detect a legacy guest is bogus, need to think what is
the best way to handle this.

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

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

[Si-Wei Liu]

On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>
>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>
>>>>> Thanks
>>>> And also:
>>>>
>>>> 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”.
>>>>
>>>>
>>>> so accessing config space before FEATURES_OK is a spec violation, right?
>>> It is, but it's not relevant to what this commit tries to address. I
>>> thought the legacy guest still needs to be supported.
>>>
>>> Having said, a separate patch has to be posted to fix the guest driver
>>> issue where this discrepancy is introduced to virtnet_validate() (since
>>> commit fe36cbe067). But it's not technically related to this patch.
>>>
>>> -Siwei
>>
>> I think it's a bug to read config space in validate, we should move it to
>> virtnet_probe().
>>
>> Thanks
> I take it back, reading but not writing seems to be explicitly allowed by spec.
> So our way to detect a legacy guest is bogus, need to think what is
> the best way to handle this.
Then maybe revert commit fe36cbe067 and friends, and have QEMU detect 
legacy guest? Supposedly only config space write access needs to be 
guarded before setting FEATURES_OK.

-Siwie

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

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

[Jason Wang]

On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
>
>
> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>
>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>
>>>>>> Thanks
>>>>> And also:
>>>>>
>>>>> 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”.
>>>>>
>>>>>
>>>>> so accessing config space before FEATURES_OK is a spec violation, 
>>>>> right?
>>>> It is, but it's not relevant to what this commit tries to address. I
>>>> thought the legacy guest still needs to be supported.
>>>>
>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>> issue where this discrepancy is introduced to virtnet_validate() 
>>>> (since
>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>
>>>> -Siwei
>>>
>>> I think it's a bug to read config space in validate, we should move 
>>> it to
>>> virtnet_probe().
>>>
>>> Thanks
>> I take it back, reading but not writing seems to be explicitly 
>> allowed by spec.
>> So our way to detect a legacy guest is bogus, need to think what is
>> the best way to handle this.
> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect 
> legacy guest? Supposedly only config space write access needs to be 
> guarded before setting FEATURES_OK.


I agree. My understanding is that all vDPA must be modern device (since 
VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.

Thanks


>
> -Siwie
>
>>>>>
>>>>>>> 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@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)
>

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
> 
> 
> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > 
> > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > 
> > > > > > Thanks
> > > > > And also:
> > > > > 
> > > > > 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”.
> > > > > 
> > > > > 
> > > > > so accessing config space before FEATURES_OK is a spec violation, right?
> > > > It is, but it's not relevant to what this commit tries to address. I
> > > > thought the legacy guest still needs to be supported.
> > > > 
> > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > issue where this discrepancy is introduced to virtnet_validate() (since
> > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > 
> > > > -Siwei
> > > 
> > > I think it's a bug to read config space in validate, we should move it to
> > > virtnet_probe().
> > > 
> > > Thanks
> > I take it back, reading but not writing seems to be explicitly allowed by spec.
> > So our way to detect a legacy guest is bogus, need to think what is
> > the best way to handle this.
> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
> guest? Supposedly only config space write access needs to be guarded before
> setting FEATURES_OK.
> 
> -Siwie

Detecting it isn't enough though, we will need a new ioctl to notify
the kernel that it's a legacy guest. Ugh :(


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

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> 
> On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > 
> > 
> > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > 
> > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > 
> > > > > > > Thanks
> > > > > > And also:
> > > > > > 
> > > > > > 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”.
> > > > > > 
> > > > > > 
> > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > violation, right?
> > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > thought the legacy guest still needs to be supported.
> > > > > 
> > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > issue where this discrepancy is introduced to
> > > > > virtnet_validate() (since
> > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > 
> > > > > -Siwei
> > > > 
> > > > I think it's a bug to read config space in validate, we should
> > > > move it to
> > > > virtnet_probe().
> > > > 
> > > > Thanks
> > > I take it back, reading but not writing seems to be explicitly
> > > allowed by spec.
> > > So our way to detect a legacy guest is bogus, need to think what is
> > > the best way to handle this.
> > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > legacy guest? Supposedly only config space write access needs to be
> > guarded before setting FEATURES_OK.
> 
> 
> I agree. My understanding is that all vDPA must be modern device (since
> VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> 
> Thanks

Well mlx5 has some code to handle legacy guests ...
Eli, could you comment? Is that support unused right now?


> 
> > 
> > -Siwie
> > 
> > > > > > 
> > > > > > > > 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@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)
> > 

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

[Jason Wang]

On 2021/2/24 1:17 下午, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
>> On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
>>>
>>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>> And also:
>>>>>>>
>>>>>>> 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”.
>>>>>>>
>>>>>>>
>>>>>>> so accessing config space before FEATURES_OK is a spec
>>>>>>> violation, right?
>>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>>> thought the legacy guest still needs to be supported.
>>>>>>
>>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>>> issue where this discrepancy is introduced to
>>>>>> virtnet_validate() (since
>>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>>
>>>>>> -Siwei
>>>>> I think it's a bug to read config space in validate, we should
>>>>> move it to
>>>>> virtnet_probe().
>>>>>
>>>>> Thanks
>>>> I take it back, reading but not writing seems to be explicitly
>>>> allowed by spec.
>>>> So our way to detect a legacy guest is bogus, need to think what is
>>>> the best way to handle this.
>>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
>>> legacy guest? Supposedly only config space write access needs to be
>>> guarded before setting FEATURES_OK.
>>
>> I agree. My understanding is that all vDPA must be modern device (since
>> VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
>>
>> Thanks
> Well mlx5 has some code to handle legacy guests ...


My understanding is that, even if mlx5 is modern device it can still 
suppot legacy guests since the device saw by guest is emulated by Qemu. 
Qemu can just present a transitional device to guest, but negotiate 
VIRTIO_F_ACCESS_PLATFORM. (Actually this is what has been done now).

Thanks


> Eli, could you comment? Is that support unused right now?
>
>
>>> -Siwie
>>>
>>>>>>>>> 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@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)

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

[Jason Wang]

On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote:
> On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
>>
>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>>
>>>>>>> Thanks
>>>>>> And also:
>>>>>>
>>>>>> 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”.
>>>>>>
>>>>>>
>>>>>> so accessing config space before FEATURES_OK is a spec violation, right?
>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>> thought the legacy guest still needs to be supported.
>>>>>
>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>> issue where this discrepancy is introduced to virtnet_validate() (since
>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>
>>>>> -Siwei
>>>> I think it's a bug to read config space in validate, we should move it to
>>>> virtnet_probe().
>>>>
>>>> Thanks
>>> I take it back, reading but not writing seems to be explicitly allowed by spec.
>>> So our way to detect a legacy guest is bogus, need to think what is
>>> the best way to handle this.
>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
>> guest? Supposedly only config space write access needs to be guarded before
>> setting FEATURES_OK.
>>
>> -Siwie
> Detecting it isn't enough though, we will need a new ioctl to notify
> the kernel that it's a legacy guest. Ugh :(


I'm not sure I get this, how can we know if there's a legacy driver 
before set_features()?

And I wonder what will hapeen if we just revert the set_features(0)?

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

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

[Eli Cohen]

On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> > 
> > On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > > 
> > > 
> > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > 
> > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > And also:
> > > > > > > 
> > > > > > > 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”.
> > > > > > > 
> > > > > > > 
> > > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > > violation, right?
> > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > thought the legacy guest still needs to be supported.
> > > > > > 
> > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > issue where this discrepancy is introduced to
> > > > > > virtnet_validate() (since
> > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > 
> > > > > > -Siwei
> > > > > 
> > > > > I think it's a bug to read config space in validate, we should
> > > > > move it to
> > > > > virtnet_probe().
> > > > > 
> > > > > Thanks
> > > > I take it back, reading but not writing seems to be explicitly
> > > > allowed by spec.
> > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > the best way to handle this.
> > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > > legacy guest? Supposedly only config space write access needs to be
> > > guarded before setting FEATURES_OK.
> > 
> > 
> > I agree. My understanding is that all vDPA must be modern device (since
> > VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> > 
> > Thanks
> 
> Well mlx5 has some code to handle legacy guests ...
> Eli, could you comment? Is that support unused right now?
> 

If you mean support for version 1.0, well the knob is there but it's not
set in the firmware I use. Note sure if we will support this.

> 
> > 
> > > 
> > > -Siwie
> > > 
> > > > > > > 
> > > > > > > > > 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@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)
> > > 
> 

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 02:04:36PM +0800, Jason Wang wrote:
> 
> On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote:
> > On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
> > > 
> > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > And also:
> > > > > > > 
> > > > > > > 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”.
> > > > > > > 
> > > > > > > 
> > > > > > > so accessing config space before FEATURES_OK is a spec violation, right?
> > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > thought the legacy guest still needs to be supported.
> > > > > > 
> > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > issue where this discrepancy is introduced to virtnet_validate() (since
> > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > 
> > > > > > -Siwei
> > > > > I think it's a bug to read config space in validate, we should move it to
> > > > > virtnet_probe().
> > > > > 
> > > > > Thanks
> > > > I take it back, reading but not writing seems to be explicitly allowed by spec.
> > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > the best way to handle this.
> > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
> > > guest? Supposedly only config space write access needs to be guarded before
> > > setting FEATURES_OK.
> > > 
> > > -Siwie
> > Detecting it isn't enough though, we will need a new ioctl to notify
> > the kernel that it's a legacy guest. Ugh :(
> 
> 
> I'm not sure I get this, how can we know if there's a legacy driver before
> set_features()?

qemu knows for sure. It does not communicate this information to the
kernel right now unfortunately.

> And I wonder what will hapeen if we just revert the set_features(0)?
> 
> 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@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)

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 08:45:20AM +0200, Eli Cohen wrote:
> On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> > > 
> > > On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > > > 
> > > > 
> > > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > > 
> > > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > > 
> > > > > > > > > Thanks
> > > > > > > > And also:
> > > > > > > > 
> > > > > > > > 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”.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > > > violation, right?
> > > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > > thought the legacy guest still needs to be supported.
> > > > > > > 
> > > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > > issue where this discrepancy is introduced to
> > > > > > > virtnet_validate() (since
> > > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > > 
> > > > > > > -Siwei
> > > > > > 
> > > > > > I think it's a bug to read config space in validate, we should
> > > > > > move it to
> > > > > > virtnet_probe().
> > > > > > 
> > > > > > Thanks
> > > > > I take it back, reading but not writing seems to be explicitly
> > > > > allowed by spec.
> > > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > > the best way to handle this.
> > > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > > > legacy guest? Supposedly only config space write access needs to be
> > > > guarded before setting FEATURES_OK.
> > > 
> > > 
> > > I agree. My understanding is that all vDPA must be modern device (since
> > > VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> > > 
> > > Thanks
> > 
> > Well mlx5 has some code to handle legacy guests ...
> > Eli, could you comment? Is that support unused right now?
> > 
> 
> If you mean support for version 1.0, well the knob is there but it's not
> set in the firmware I use. Note sure if we will support this.

Hmm you mean it's legacy only right now?
Well at some point you will want advanced goodies like RSS
and all that is gated on 1.0 ;)

> > 
> > > 
> > > > 
> > > > -Siwie
> > > > 
> > > > > > > > 
> > > > > > > > > > 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@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)
> > > > 
> > 

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

[Jason Wang]

On 2021/2/24 2:46 下午, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 02:04:36PM +0800, Jason Wang wrote:
>> On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote:
>>> On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
>>>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>> And also:
>>>>>>>>
>>>>>>>> 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”.
>>>>>>>>
>>>>>>>>
>>>>>>>> so accessing config space before FEATURES_OK is a spec violation, right?
>>>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>>>> thought the legacy guest still needs to be supported.
>>>>>>>
>>>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>>>> issue where this discrepancy is introduced to virtnet_validate() (since
>>>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>>>
>>>>>>> -Siwei
>>>>>> I think it's a bug to read config space in validate, we should move it to
>>>>>> virtnet_probe().
>>>>>>
>>>>>> Thanks
>>>>> I take it back, reading but not writing seems to be explicitly allowed by spec.
>>>>> So our way to detect a legacy guest is bogus, need to think what is
>>>>> the best way to handle this.
>>>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
>>>> guest? Supposedly only config space write access needs to be guarded before
>>>> setting FEATURES_OK.
>>>>
>>>> -Siwie
>>> Detecting it isn't enough though, we will need a new ioctl to notify
>>> the kernel that it's a legacy guest. Ugh :(
>>
>> I'm not sure I get this, how can we know if there's a legacy driver before
>> set_features()?
> qemu knows for sure. It does not communicate this information to the
> kernel right now unfortunately.


I may miss something, but I still don't get how the new ioctl is 
supposed to work.

Thanks


>
>> And I wonder what will hapeen if we just revert the set_features(0)?
>>
>> 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@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)

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

[Jason Wang]

On 2021/2/24 2:47 下午, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 08:45:20AM +0200, Eli Cohen wrote:
>> On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
>>> On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
>>>> On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
>>>>>
>>>>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>>>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>> And also:
>>>>>>>>>
>>>>>>>>> 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”.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> so accessing config space before FEATURES_OK is a spec
>>>>>>>>> violation, right?
>>>>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>>>>> thought the legacy guest still needs to be supported.
>>>>>>>>
>>>>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>>>>> issue where this discrepancy is introduced to
>>>>>>>> virtnet_validate() (since
>>>>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>>>>
>>>>>>>> -Siwei
>>>>>>> I think it's a bug to read config space in validate, we should
>>>>>>> move it to
>>>>>>> virtnet_probe().
>>>>>>>
>>>>>>> Thanks
>>>>>> I take it back, reading but not writing seems to be explicitly
>>>>>> allowed by spec.
>>>>>> So our way to detect a legacy guest is bogus, need to think what is
>>>>>> the best way to handle this.
>>>>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
>>>>> legacy guest? Supposedly only config space write access needs to be
>>>>> guarded before setting FEATURES_OK.
>>>>
>>>> I agree. My understanding is that all vDPA must be modern device (since
>>>> VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
>>>>
>>>> Thanks
>>> Well mlx5 has some code to handle legacy guests ...
>>> Eli, could you comment? Is that support unused right now?
>>>
>> If you mean support for version 1.0, well the knob is there but it's not
>> set in the firmware I use. Note sure if we will support this.
> Hmm you mean it's legacy only right now?
> Well at some point you will want advanced goodies like RSS
> and all that is gated on 1.0 ;)


So if my understanding is correct the device/firmware is legacy but 
require VIRTIO_F_ACCESS_PLATFORM semanic? Looks like a spec violation?

Thanks


>
>>>>> -Siwie
>>>>>
>>>>>>>>>>> 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@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)

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 02:55:13PM +0800, Jason Wang wrote:
> 
> On 2021/2/24 2:47 下午, Michael S. Tsirkin wrote:
> > On Wed, Feb 24, 2021 at 08:45:20AM +0200, Eli Cohen wrote:
> > > On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> > > > > On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > > > > > 
> > > > > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > > > > 
> > > > > > > > > > > Thanks
> > > > > > > > > > And also:
> > > > > > > > > > 
> > > > > > > > > > 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”.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > > > > > violation, right?
> > > > > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > > > > thought the legacy guest still needs to be supported.
> > > > > > > > > 
> > > > > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > > > > issue where this discrepancy is introduced to
> > > > > > > > > virtnet_validate() (since
> > > > > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > > > > 
> > > > > > > > > -Siwei
> > > > > > > > I think it's a bug to read config space in validate, we should
> > > > > > > > move it to
> > > > > > > > virtnet_probe().
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > I take it back, reading but not writing seems to be explicitly
> > > > > > > allowed by spec.
> > > > > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > > > > the best way to handle this.
> > > > > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > > > > > legacy guest? Supposedly only config space write access needs to be
> > > > > > guarded before setting FEATURES_OK.
> > > > > 
> > > > > I agree. My understanding is that all vDPA must be modern device (since
> > > > > VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> > > > > 
> > > > > Thanks
> > > > Well mlx5 has some code to handle legacy guests ...
> > > > Eli, could you comment? Is that support unused right now?
> > > > 
> > > If you mean support for version 1.0, well the knob is there but it's not
> > > set in the firmware I use. Note sure if we will support this.
> > Hmm you mean it's legacy only right now?
> > Well at some point you will want advanced goodies like RSS
> > and all that is gated on 1.0 ;)
> 
> 
> So if my understanding is correct the device/firmware is legacy but require
> VIRTIO_F_ACCESS_PLATFORM semanic? Looks like a spec violation?
> 
> Thanks

Legacy mode description is the spec is non-normative. As such as long as
guests work, they work ;)

> 
> > 
> > > > > > -Siwie
> > > > > > 
> > > > > > > > > > > > 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@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)

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

[Eli Cohen]

On Wed, Feb 24, 2021 at 02:55:13PM +0800, Jason Wang wrote:
> 
> On 2021/2/24 2:47 下午, Michael S. Tsirkin wrote:
> > On Wed, Feb 24, 2021 at 08:45:20AM +0200, Eli Cohen wrote:
> > > On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> > > > > On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > > > > > 
> > > > > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > > > > 
> > > > > > > > > > > Thanks
> > > > > > > > > > And also:
> > > > > > > > > > 
> > > > > > > > > > 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”.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > > > > > violation, right?
> > > > > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > > > > thought the legacy guest still needs to be supported.
> > > > > > > > > 
> > > > > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > > > > issue where this discrepancy is introduced to
> > > > > > > > > virtnet_validate() (since
> > > > > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > > > > 
> > > > > > > > > -Siwei
> > > > > > > > I think it's a bug to read config space in validate, we should
> > > > > > > > move it to
> > > > > > > > virtnet_probe().
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > I take it back, reading but not writing seems to be explicitly
> > > > > > > allowed by spec.
> > > > > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > > > > the best way to handle this.
> > > > > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > > > > > legacy guest? Supposedly only config space write access needs to be
> > > > > > guarded before setting FEATURES_OK.
> > > > > 
> > > > > I agree. My understanding is that all vDPA must be modern device (since
> > > > > VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> > > > > 
> > > > > Thanks
> > > > Well mlx5 has some code to handle legacy guests ...
> > > > Eli, could you comment? Is that support unused right now?
> > > > 
> > > If you mean support for version 1.0, well the knob is there but it's not
> > > set in the firmware I use. Note sure if we will support this.
> > Hmm you mean it's legacy only right now?
> > Well at some point you will want advanced goodies like RSS
> > and all that is gated on 1.0 ;)
> 
> 
> So if my understanding is correct the device/firmware is legacy but require
> VIRTIO_F_ACCESS_PLATFORM semanic? Looks like a spec violation?
> 

I am checking this with some folks here.

> Thanks
> 
> 
> > 
> > > > > > -Siwie
> > > > > > 
> > > > > > > > > > > > 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@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)
> 

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 02:53:08PM +0800, Jason Wang wrote:
> 
> On 2021/2/24 2:46 下午, Michael S. Tsirkin wrote:
> > On Wed, Feb 24, 2021 at 02:04:36PM +0800, Jason Wang wrote:
> > > On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
> > > > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > > > 
> > > > > > > > > > Thanks
> > > > > > > > > And also:
> > > > > > > > > 
> > > > > > > > > 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”.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > so accessing config space before FEATURES_OK is a spec violation, right?
> > > > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > > > thought the legacy guest still needs to be supported.
> > > > > > > > 
> > > > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > > > issue where this discrepancy is introduced to virtnet_validate() (since
> > > > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > > > 
> > > > > > > > -Siwei
> > > > > > > I think it's a bug to read config space in validate, we should move it to
> > > > > > > virtnet_probe().
> > > > > > > 
> > > > > > > Thanks
> > > > > > I take it back, reading but not writing seems to be explicitly allowed by spec.
> > > > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > > > the best way to handle this.
> > > > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
> > > > > guest? Supposedly only config space write access needs to be guarded before
> > > > > setting FEATURES_OK.
> > > > > 
> > > > > -Siwie
> > > > Detecting it isn't enough though, we will need a new ioctl to notify
> > > > the kernel that it's a legacy guest. Ugh :(
> > > 
> > > I'm not sure I get this, how can we know if there's a legacy driver before
> > > set_features()?
> > qemu knows for sure. It does not communicate this information to the
> > kernel right now unfortunately.
> 
> 
> I may miss something, but I still don't get how the new ioctl is supposed to
> work.
> 
> Thanks



Basically on first guest access QEMU would tell kernel whether
guest is using the legacy or the modern interface.
E.g. virtio_pci_config_read/virtio_pci_config_write will call ioctl(ENABLE_LEGACY, 1)
while virtio_pci_common_read will call ioctl(ENABLE_LEGACY, 0)

Or maybe we just add GET_CONFIG_MODERN and GET_CONFIG_LEGACY and
call the correct ioctl ... there are many ways to build this API.

> 
> > 
> > > And I wonder what will hapeen if we just revert the set_features(0)?
> > > 
> > > 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@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)

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

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 04:26:43PM +0800, Jason Wang wrote:
>     Basically on first guest access QEMU would tell kernel whether
>     guest is using the legacy or the modern interface.
>     E.g. virtio_pci_config_read/virtio_pci_config_write will call ioctl(ENABLE_LEGACY, 1)
>     while virtio_pci_common_read will call ioctl(ENABLE_LEGACY, 0)
> 
> 
> But this trick work only for PCI I think?
> 
> Thanks

ccw has a revision it can check. mmio does not have transitional devices
at all.

-- 
MST

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

[Jason Wang]

On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 18:31:07 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>> Jason Wang <jasowang@redhat.com> 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?
>>> I think both failing or not accepting the feature can be argued to make
>>> sense: "the device presented us with a mtu size that does not make
>>> sense" would point to failing, "we cannot work with the mtu size that
>>> the device presented us" would point to not negotiating the feature.
>>>   
>>>>   
>>>>>            }
>>>>>
>>>>>            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.
>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>> has offered the feature";
>>
>> For me this part is ok since it clarify that it's the driver that set
>> the bit.
>>
>>
>>
>>> during normal usage, it means "the feature
>>> has been negotiated".
>> /?
>>
>> It looks to me the feature negotiation is done only after device set
>> FEATURES_OK, or FEATURES_OK could be read from device status?
> I'd consider feature negotiation done when the driver reads FEATURES_OK
> back from the status.


I agree.


>
>>
>>>    (This is a bit fuzzy for legacy mode.)
> ...because legacy does not have FEATURES_OK.
>    
>>
>> 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.

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.


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


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


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

Thanks


>

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

[Jason Wang]

On 2021/2/24 4:43 下午, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 04:26:43PM +0800, Jason Wang wrote:
>>      Basically on first guest access QEMU would tell kernel whether
>>      guest is using the legacy or the modern interface.
>>      E.g. virtio_pci_config_read/virtio_pci_config_write will call ioctl(ENABLE_LEGACY, 1)
>>      while virtio_pci_common_read will call ioctl(ENABLE_LEGACY, 0)
>>
>>
>> But this trick work only for PCI I think?
>>
>> Thanks
> ccw has a revision it can check. mmio does not have transitional devices
> at all.


Ok, then we can do the workaround in the qemu, isn't it?

Thanks

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

[Jason Wang]

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

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

[Cornelia Huck]

On Wed, 24 Feb 2021 17:29:07 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > On Tue, 23 Feb 2021 18:31:07 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> >> On 2021/2/23 6:04 下午, Cornelia Huck wrote:  
> >>> On Tue, 23 Feb 2021 17:46:20 +0800
> >>> Jason Wang <jasowang@redhat.com> 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?  
> >>> I think both failing or not accepting the feature can be argued to make
> >>> sense: "the device presented us with a mtu size that does not make
> >>> sense" would point to failing, "we cannot work with the mtu size that
> >>> the device presented us" would point to not negotiating the feature.
> >>>     
> >>>>     
> >>>>>            }
> >>>>>
> >>>>>            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.  
> >>> Before FEATURES_OK is set by the driver, I guess it means "the device
> >>> has offered the feature";  
> >>
> >> For me this part is ok since it clarify that it's the driver that set
> >> the bit.
> >>
> >>
> >>  
> >>> during normal usage, it means "the feature
> >>> has been negotiated".  
> >> /?
> >>
> >> It looks to me the feature negotiation is done only after device set
> >> FEATURES_OK, or FEATURES_OK could be read from device status?  
> > I'd consider feature negotiation done when the driver reads FEATURES_OK
> > back from the status.  
> 
> 
> I agree.
> 
> 
> >  
> >>  
> >>>    (This is a bit fuzzy for legacy mode.)  
> > ...because legacy does not have FEATURES_OK.
> >      
> >>
> >> 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?

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

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

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

[Eli Cohen]

On Wed, Feb 24, 2021 at 02:12:01AM -0500, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 02:55:13PM +0800, Jason Wang wrote:
> > 
> > On 2021/2/24 2:47 下午, Michael S. Tsirkin wrote:
> > > On Wed, Feb 24, 2021 at 08:45:20AM +0200, Eli Cohen wrote:
> > > > On Wed, Feb 24, 2021 at 12:17:58AM -0500, Michael S. Tsirkin wrote:
> > > > > On Wed, Feb 24, 2021 at 11:20:01AM +0800, Jason Wang wrote:
> > > > > > On 2021/2/24 3:35 上午, Si-Wei Liu wrote:
> > > > > > > 
> > > > > > > On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
> > > > > > > > On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
> > > > > > > > > On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
> > > > > > > > > > On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks
> > > > > > > > > > > And also:
> > > > > > > > > > > 
> > > > > > > > > > > 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”.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > so accessing config space before FEATURES_OK is a spec
> > > > > > > > > > > violation, right?
> > > > > > > > > > It is, but it's not relevant to what this commit tries to address. I
> > > > > > > > > > thought the legacy guest still needs to be supported.
> > > > > > > > > > 
> > > > > > > > > > Having said, a separate patch has to be posted to fix the guest driver
> > > > > > > > > > issue where this discrepancy is introduced to
> > > > > > > > > > virtnet_validate() (since
> > > > > > > > > > commit fe36cbe067). But it's not technically related to this patch.
> > > > > > > > > > 
> > > > > > > > > > -Siwei
> > > > > > > > > I think it's a bug to read config space in validate, we should
> > > > > > > > > move it to
> > > > > > > > > virtnet_probe().
> > > > > > > > > 
> > > > > > > > > Thanks
> > > > > > > > I take it back, reading but not writing seems to be explicitly
> > > > > > > > allowed by spec.
> > > > > > > > So our way to detect a legacy guest is bogus, need to think what is
> > > > > > > > the best way to handle this.
> > > > > > > Then maybe revert commit fe36cbe067 and friends, and have QEMU detect
> > > > > > > legacy guest? Supposedly only config space write access needs to be
> > > > > > > guarded before setting FEATURES_OK.
> > > > > > 
> > > > > > I agree. My understanding is that all vDPA must be modern device (since
> > > > > > VIRITO_F_ACCESS_PLATFORM is mandated) instead of transitional device.
> > > > > > 
> > > > > > Thanks
> > > > > Well mlx5 has some code to handle legacy guests ...
> > > > > Eli, could you comment? Is that support unused right now?
> > > > > 
> > > > If you mean support for version 1.0, well the knob is there but it's not
> > > > set in the firmware I use. Note sure if we will support this.
> > > Hmm you mean it's legacy only right now?
> > > Well at some point you will want advanced goodies like RSS
> > > and all that is gated on 1.0 ;)
> > 

Guys sorry, I checked again, the firmware capability is set and so is
VIRTIO_F_VERSION_1.

> > 
> > So if my understanding is correct the device/firmware is legacy but require
> > VIRTIO_F_ACCESS_PLATFORM semanic? Looks like a spec violation?
> > 
> > Thanks
> 
> Legacy mode description is the spec is non-normative. As such as long as
> guests work, they work ;)
> 
> > 
> > > 
> > > > > > > -Siwie
> > > > > > > 
> > > > > > > > > > > > > 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@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)
> 

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

[Si-Wei Liu]

On 2/23/2021 9:04 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
>>
>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, 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?
>>>>>>>
>>>>>>> Thanks
>>>>>> And also:
>>>>>>
>>>>>> 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”.
>>>>>>
>>>>>>
>>>>>> so accessing config space before FEATURES_OK is a spec violation, right?
>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>> thought the legacy guest still needs to be supported.
>>>>>
>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>> issue where this discrepancy is introduced to virtnet_validate() (since
>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>
>>>>> -Siwei
>>>> I think it's a bug to read config space in validate, we should move it to
>>>> virtnet_probe().
>>>>
>>>> Thanks
>>> I take it back, reading but not writing seems to be explicitly allowed by spec.
>>> So our way to detect a legacy guest is bogus, need to think what is
>>> the best way to handle this.
>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
>> guest? Supposedly only config space write access needs to be guarded before
>> setting FEATURES_OK.
>>
>> -Siwie
> Detecting it isn't enough though, we will need a new ioctl to notify
> the kernel that it's a legacy guest. Ugh :(
Well, although I think adding an ioctl is doable, may I know what the 
use case there will be for kernel to leverage such info directly? Is 
there a case QEMU can't do with dedicate ioctls later if there's indeed 
differentiation (legacy v.s. modern) needed?

One of the reason I asked is if this ioctl becomes a mandate for 
vhost-vdpa kernel. QEMU would reject initialize vhost-vdpa if doesn't 
see this ioctl coming?

If it's optional, suppose the kernel may need it only when it becomes 
necessary?

Thanks,
-Siwei


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

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

[Jason Wang]

On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> On Wed, 24 Feb 2021 17:29:07 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
>>> On Tue, 23 Feb 2021 18:31:07 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>   
>>>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>>>> Jason Wang <jasowang@redhat.com> 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?
>>>>> I think both failing or not accepting the feature can be argued to make
>>>>> sense: "the device presented us with a mtu size that does not make
>>>>> sense" would point to failing, "we cannot work with the mtu size that
>>>>> the device presented us" would point to not negotiating the feature.
>>>>>      
>>>>>>      
>>>>>>>             }
>>>>>>>
>>>>>>>             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.
>>>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>>>> has offered the feature";
>>>> For me this part is ok since it clarify that it's the driver that set
>>>> the bit.
>>>>
>>>>
>>>>   
>>>>> during normal usage, it means "the feature
>>>>> has been negotiated".
>>>> /?
>>>>
>>>> It looks to me the feature negotiation is done only after device set
>>>> FEATURES_OK, or FEATURES_OK could be read from device status?
>>> I'd consider feature negotiation done when the driver reads FEATURES_OK
>>> back from the status.
>>
>> I agree.
>>
>>
>>>   
>>>>   
>>>>>     (This is a bit fuzzy for legacy mode.)
>>> ...because legacy does not have FEATURES_OK.
>>>       
>>>> 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);
         }

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


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


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

Thanks

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@redhat.com> wrote:

> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> >> On 2021/2/23 6:58 下午, Cornelia Huck wrote:  
> >>> On Tue, 23 Feb 2021 18:31:07 +0800
> >>> Jason Wang <jasowang@redhat.com> 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.

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

[Michael S. Tsirkin]

On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
> 
> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > > > On Tue, 23 Feb 2021 18:31:07 +0800
> > > > Jason Wang <jasowang@redhat.com> wrote:
> > > > > On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > > > > > On Tue, 23 Feb 2021 17:46:20 +0800
> > > > > > Jason Wang <jasowang@redhat.com> 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?
> > > > > > I think both failing or not accepting the feature can be argued to make
> > > > > > sense: "the device presented us with a mtu size that does not make
> > > > > > sense" would point to failing, "we cannot work with the mtu size that
> > > > > > the device presented us" would point to not negotiating the feature.
> > > > > > > >             }
> > > > > > > > 
> > > > > > > >             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.
> > > > > > Before FEATURES_OK is set by the driver, I guess it means "the device
> > > > > > has offered the feature";
> > > > > For me this part is ok since it clarify that it's the driver that set
> > > > > the bit.
> > > > > 
> > > > > 
> > > > > > during normal usage, it means "the feature
> > > > > > has been negotiated".
> > > > > /?
> > > > > 
> > > > > It looks to me the feature negotiation is done only after device set
> > > > > FEATURES_OK, or FEATURES_OK could be read from device status?
> > > > I'd consider feature negotiation done when the driver reads FEATURES_OK
> > > > back from the status.
> > > 
> > > I agree.
> > > 
> > > 
> > > > > >     (This is a bit fuzzy for legacy mode.)
> > > > ...because legacy does not have FEATURES_OK.
> > > > > 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);
>         }
> 
> So it's probably too late to fix the driver.
> 

Confused. What is wrong with the above? It never reads the
field unless the feature has been offered by device.


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

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

[Si-Wei Liu]

Hi Michael,

Are you okay to live without this ioctl for now? I think QEMU is the one 
that needs to be fixed and will have to be made legacy guest aware. I 
think the kernel can just honor the feature negotiation result done by 
QEMU and do as what's told to.Will you agree?

If it's fine, I would proceed to reverting commit fe36cbe067 and related 
code in question from the kernel.

Thanks,
-Siwei

On 2/24/2021 10:24 AM, Si-Wei Liu wrote:
>> Detecting it isn't enough though, we will need a new ioctl to notify
>> the kernel that it's a legacy guest. Ugh :(
> Well, although I think adding an ioctl is doable, may I know what the 
> use case there will be for kernel to leverage such info directly? Is 
> there a case QEMU can't do with dedicate ioctls later if there's 
> indeed differentiation (legacy v.s. modern) needed?
>
> One of the reason I asked is if this ioctl becomes a mandate for 
> vhost-vdpa kernel. QEMU would reject initialize vhost-vdpa if doesn't 
> see this ioctl coming?
>
> If it's optional, suppose the kernel may need it only when it becomes 
> necessary?
>
> Thanks,
> -Siwei

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

[Jason Wang]

On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote:
> On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
>> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
>>> On Wed, 24 Feb 2021 17:29:07 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
>>>>> On Tue, 23 Feb 2021 18:31:07 +0800
>>>>> Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>>>>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>>>>>> Jason Wang <jasowang@redhat.com> 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?
>>>>>>> I think both failing or not accepting the feature can be argued to make
>>>>>>> sense: "the device presented us with a mtu size that does not make
>>>>>>> sense" would point to failing, "we cannot work with the mtu size that
>>>>>>> the device presented us" would point to not negotiating the feature.
>>>>>>>>>              }
>>>>>>>>>
>>>>>>>>>              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.
>>>>>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>>>>>> has offered the feature";
>>>>>> For me this part is ok since it clarify that it's the driver that set
>>>>>> the bit.
>>>>>>
>>>>>>
>>>>>>> during normal usage, it means "the feature
>>>>>>> has been negotiated".
>>>>>> /?
>>>>>>
>>>>>> It looks to me the feature negotiation is done only after device set
>>>>>> FEATURES_OK, or FEATURES_OK could be read from device status?
>>>>> I'd consider feature negotiation done when the driver reads FEATURES_OK
>>>>> back from the status.
>>>> I agree.
>>>>
>>>>
>>>>>>>      (This is a bit fuzzy for legacy mode.)
>>>>> ...because legacy does not have FEATURES_OK.
>>>>>> 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);
>>          }
>>
>> So it's probably too late to fix the driver.
>>
> Confused. What is wrong with the above? It never reads the
> field unless the feature has been offered by device.


So the spec said:

"

The following driver-read-only field, max_virtqueue_pairs only exists if 
VIRTIO_NET_F_MQ is set.

"

If I read this correctly, there will be no max_virtqueue_pairs field if 
the VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() 
violates what spec said.

Thanks


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