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

[Eli Cohen <elic@nvidia.com>]

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