Si-Wei Liu
2023-Feb-06 04:53 UTC
[PATCH v3 2/6] vdpa: conditionally read STATUS in config space
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 at oracle.com> >> Reviewed-by: Parav Pandit <parav at 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_STATUSIn 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); >> ? }