在 2022/7/28 08:56, Si-Wei Liu 写道:
On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:
On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
Sorry to chime in late in the game. For some reason I couldn't get
to most emails for this discussion (I only subscribed to the
virtualization list), while I was taking off amongst the past few
weeks.
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.
I have another patch in this series introduces device_features and
will report device_features to the userspace even features
negotiation not done. Because the spec says we should allow driver
access the config space before FEATURES_OK.
The config space can be accessed by guest before features_ok doesn't
necessarily mean the value is valid.
It's valid as long as the device offers the feature:
"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."
You may want to double check with Michael for what he quoted earlier:
Nope:
2.5.1 Driver Requirements: Device Configuration Space
...
For optional configuration space fields, the driver MUST check that the corresponding feature is offered
before accessing that part of the configuration space.
and how many driver bugs taking wrong assumption of the validity of
config space field without features_ok. I am not sure what use case
you want to expose config resister values for before features_ok, if
it's mostly for live migration I guess it's probably heading a wrong
direction.
I guess it's not for migration. For migration, a provision with the
correct features/capability would be sufficient.
Thanks
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.
DEV - specifies the vdpa device to show its configuration. If this
argument is omitted all devices configuration is listed.
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.
I am not sure getting your points. vDPA now reports device feature
bits(device_features) and negotiated feature bits(driver_features),
and yes, the drivers features can be a subset of the device features;
and the vDPA device features can be a subset of the management device
features.
What I said is after unblocking the conditional check, you'd have to
handle the case for each of the vdpa attribute when feature
negotiation is not yet done: basically the register values you got
from config space via the vdpa_get_config_unlocked() call is not
considered to be valid before features_ok (per-spec). Although in some
case you may get sane value, such behavior is generally undefined. If
you desire to show just the device_features alone without any config
space field, which the device had advertised *before feature
negotiation is complete*, that'll be fine. But looks to me this is not
how patch has been implemented. Probably need some more work?
Regards,
-Siwei
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.
it's a new feature to report the device feature bits than only
negotiated features, however this patch is a must, or it will block
the device feature bits reporting. but I agree, the fix tag is not a
must.
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
