diff mbox series

[v3,2/6] vdpa: conditionally read STATUS in config space

Message ID 1675400523-12519-3-git-send-email-si-wei.liu@oracle.com
State New
Headers
Series features provisioning fixes and mlx5_vdpa support |

Commit Message

Si-Wei Liu Feb. 3, 2023, 5:01 a.m. UTC 
  The spec says:
    status only exists if VIRTIO_NET_F_STATUS is set

Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
STATUS conditionally depending on the feature bits.

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
 drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Eli Cohen Feb. 5, 2023, 8:26 a.m. UTC | #1 
On 03/02/2023 7:01, Si-Wei Liu wrote:
> The spec says:
>      status only exists if VIRTIO_NET_F_STATUS is set
>
> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
> STATUS conditionally depending on the feature bits.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3a82ca78..21c8aa3 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct sk_buff *msg, u64 features,
>   			sizeof(config->mac), config->mac);
>   }
>   
> +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
> +					   const struct virtio_net_config *config)
> +{
> +	u16 val_u16;
> +
> +	if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
> +		return 0;
Instead of returning 0 here, it would be better to explicitly put 0 in 
the message field for

VDPA_ATTR_DEV_NET_STATUS

> +
> +	val_u16 = __virtio16_to_cpu(true, config->status);
> +	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
> +}
> +
>   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
>   {
>   	struct virtio_net_config config = {};
>   	u64 features_device;
> -	u16 val_u16;
>   
>   	vdev->config->get_config(vdev, 0, &config, sizeof(config));
>   
> -	val_u16 = __virtio16_to_cpu(true, config.status);
> -	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> -		return -EMSGSIZE;
> -
>   	features_device = vdev->config->get_device_features(vdev);
>   
>   	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, features_device,
> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>   	if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>   		return -EMSGSIZE;
>   
> +	if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
> +		return -EMSGSIZE;
> +
>   	return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>   }
>
Si-Wei Liu Feb. 6, 2023, 4:53 a.m. UTC | #2 
On 2/5/2023 12:26 AM, Eli Cohen wrote:
>
> On 03/02/2023 7:01, Si-Wei Liu wrote:
>> The spec says:
>>      status only exists if VIRTIO_NET_F_STATUS is set
>>
>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>> STATUS conditionally depending on the feature bits.
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> ---
>>   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 3a82ca78..21c8aa3 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct 
>> sk_buff *msg, u64 features,
>>               sizeof(config->mac), config->mac);
>>   }
>>   +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, 
>> u64 features,
>> +                       const struct virtio_net_config *config)
>> +{
>> +    u16 val_u16;
>> +
>> +    if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>> +        return 0;
> Instead of returning 0 here, it would be better to explicitly put 0 in 
> the message field for
>
> VDPA_ATTR_DEV_NET_STATUS
In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC 
in dev cfg space"), the userspace must now show the config space field 
presented by the device *as-is*. If the feature bit is not offered by 
device, the relevant field will not be displayed in 'vdpa dev config' 
output. For instance, MAC address won't be shown if the MAC feature is 
not supported/offered by the device (note this has nothing to do with 
negotiated features), even though the vdpa parent may have a non-zero 
MAC address of its own. I think STATUS should not be different from MAC 
and MTU here.

Regards,
-Siwei

>
>> +
>> +    val_u16 = __virtio16_to_cpu(true, config->status);
>> +    return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>> +}
>> +
>>   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, 
>> struct sk_buff *msg)
>>   {
>>       struct virtio_net_config config = {};
>>       u64 features_device;
>> -    u16 val_u16;
>>         vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>   -    val_u16 = __virtio16_to_cpu(true, config.status);
>> -    if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>> -        return -EMSGSIZE;
>> -
>>       features_device = vdev->config->get_device_features(vdev);
>>         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, 
>> features_device,
>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct 
>> vdpa_device *vdev, struct sk_buff *ms
>>       if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>>           return -EMSGSIZE;
>>   +    if (vdpa_dev_net_status_config_fill(msg, features_device, 
>> &config))
>> +        return -EMSGSIZE;
>> +
>>       return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>>   }
Eli Cohen Feb. 6, 2023, 6:48 a.m. UTC | #3 
On 06/02/2023 6:53, Si-Wei Liu wrote:
>
>
> On 2/5/2023 12:26 AM, Eli Cohen wrote:
>>
>> On 03/02/2023 7:01, Si-Wei Liu wrote:
>>> The spec says:
>>>      status only exists if VIRTIO_NET_F_STATUS is set
>>>
>>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>>> STATUS conditionally depending on the feature bits.
>>>
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>> ---
>>>   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 3a82ca78..21c8aa3 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct 
>>> sk_buff *msg, u64 features,
>>>               sizeof(config->mac), config->mac);
>>>   }
>>>   +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, 
>>> u64 features,
>>> +                       const struct virtio_net_config *config)
>>> +{
>>> +    u16 val_u16;
>>> +
>>> +    if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>>> +        return 0;
>> Instead of returning 0 here, it would be better to explicitly put 0 
>> in the message field for
>>
>> VDPA_ATTR_DEV_NET_STATUS
> In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC 
> in dev cfg space"), the userspace must now show the config space field 
> presented by the device *as-is*. If the feature bit is not offered by 
> device, the relevant field will not be displayed in 'vdpa dev config' 
> output. For instance, MAC address won't be shown if the MAC feature is 
> not supported/offered by the device (note this has nothing to do with 
> negotiated features), even though the vdpa parent may have a non-zero 
> MAC address of its own. I think STATUS should not be different from 
> MAC and MTU here.

OK, I see your point.

Reviewed-by: Eli Cohen <elic@nvidia.com>

>
> Regards,
> -Siwei
>
>>
>>> +
>>> +    val_u16 = __virtio16_to_cpu(true, config->status);
>>> +    return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>>> +}
>>> +
>>>   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, 
>>> struct sk_buff *msg)
>>>   {
>>>       struct virtio_net_config config = {};
>>>       u64 features_device;
>>> -    u16 val_u16;
>>>         vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>   -    val_u16 = __virtio16_to_cpu(true, config.status);
>>> -    if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>> -        return -EMSGSIZE;
>>> -
>>>       features_device = vdev->config->get_device_features(vdev);
>>>         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, 
>>> features_device,
>>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct 
>>> vdpa_device *vdev, struct sk_buff *ms
>>>       if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>>>           return -EMSGSIZE;
>>>   +    if (vdpa_dev_net_status_config_fill(msg, features_device, 
>>> &config))
>>> +        return -EMSGSIZE;
>>> +
>>>       return vdpa_dev_net_mq_config_fill(msg, features_device, 
>>> &config);
>>>   }
>
diff mbox series

Patch

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3a82ca78..21c8aa3 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -843,18 +843,25 @@  static int vdpa_dev_net_mac_config_fill(struct sk_buff *msg, u64 features,
 			sizeof(config->mac), config->mac);
 }
 
+static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
+					   const struct virtio_net_config *config)
+{
+	u16 val_u16;
+
+	if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
+		return 0;
+
+	val_u16 = __virtio16_to_cpu(true, config->status);
+	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
+}
+
 static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
 {
 	struct virtio_net_config config = {};
 	u64 features_device;
-	u16 val_u16;
 
 	vdev->config->get_config(vdev, 0, &config, sizeof(config));
 
-	val_u16 = __virtio16_to_cpu(true, config.status);
-	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
-		return -EMSGSIZE;
-
 	features_device = vdev->config->get_device_features(vdev);
 
 	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, features_device,
@@ -867,6 +874,9 @@  static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
 	if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
 		return -EMSGSIZE;
 
+	if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
+		return -EMSGSIZE;
+
 	return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
 }