Si-Wei Liu
2022-Jul-28 00:56 UTC
[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space
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. 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.>> >> >> 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 at intel.com> >>>> 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 at intel.com> >>>> --- >>>> 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 at lists.linux-foundation.org >>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20220727/41b1b09e/attachment-0001.html>
Jason Wang
2022-Jul-28 02:06 UTC
[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space
? 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 at intel.com> >>>>> 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 at intel.com> >>>>> --- >>>>> 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 at lists.linux-foundation.org >>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>> >> >