It looks to me this patch is incomplete. Noted down the way in vdpa_dev_net_config_fill(), we have the following:
features = vdev->config->get_driver_features(vdev);
if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
VDPA_ATTR_PAD))
return -EMSGSIZE;
Making call to .get_driver_features() doesn't make sense when
feature negotiation isn't complete. Neither should present
negotiated_features to userspace before negotiation is done.Similarly, max_vqp through vdpa_dev_net_mq_config_fill() probably should not show before negotiation is done - it depends on driver features negotiated.
Last but not the least, this "vdpa dev config" command was not designed to display the real config space register values in the first place. Quoting the vdpa-dev(8) man page:
vdpa dev config show - Show configuration of specific device or all devices.It doesn't say anything about configuration space or register values in config space. As long as it can convey the config attribute when instantiating vDPA device instance, and more importantly, the config can be easily imported from or exported to userspace tools when trying to reconstruct vdpa instance intact on destination host for live migration, IMHO in my personal interpretation it doesn't matter what the config space may present. It may be worth while adding a new debug command to expose the real register value, but that's another story.
DEV - specifies the vdpa device to show its configuration. If this argument is omitted all devices configuration is listed.
Having said, please consider to drop the Fixes tag, as appears to me you're proposing a new feature rather than fixing a real issue.
Thanks,
-Siwei
On 7/1/2022 3:12 PM, Parav Pandit via
Virtualization wrote:
From: Zhu Lingshan <lingshan.zhu@xxxxxxxxx> Sent: Friday, July 1, 2022 9:28 AM Users may want to query the config space of a vDPA device, to choose a appropriate one for a certain guest. This means the users need to read the config space before FEATURES_OK, and the existence of config space contents does not depend on FEATURES_OK. The spec says: The device MUST allow reading of any device-specific configuration field before FEATURES_OK is set by the driver. This includes fields which are conditional on feature bits, as long as those feature bits are offered by the device. Fixes: 30ef7a8ac8a07 (vdpa: Read device configuration only if FEATURES_OK)Fix is fine, but fixes tag needs correction described below. Above commit id is 13 letters should be 12. And It should be in format Fixes: 30ef7a8ac8a0 ("vdpa: Read device configuration only if FEATURES_OK") Please use checkpatch.pl script before posting the patches to catch these errors. There is a bot that looks at the fixes tag and identifies the right kernel version to apply this fix.Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxxxx> --- drivers/vdpa/vdpa.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 9b0e39b2f022..d76b22b2f7ae 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, { u32 device_id; void *hdr; - u8 status; int err; down_read(&vdev->cf_lock); - status = vdev->config->get_status(vdev); - if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { - NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed"); - err = -EAGAIN; - goto out; - } - hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, VDPA_CMD_DEV_CONFIG_GET); if (!hdr) { -- 2.31.1_______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
