Messages in this thread | | | Date | Mon, 6 Feb 2023 08:48:36 +0200 | Subject | Re: [PATCH v3 2/6] vdpa: conditionally read STATUS in config space | From | Eli Cohen <> |
| |
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); >>> } >
| |