Si-Wei Liu
2022-Aug-01 22:58 UTC
[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space
On 8/1/2022 3:53 PM, Si-Wei Liu wrote:> > > On 7/31/2022 9:44 PM, Jason Wang wrote: >> >> ? 2022/7/30 04:55, Si-Wei Liu ??: >>> >>> >>> On 7/28/2022 7:04 PM, Zhu, Lingshan wrote: >>>> >>>> >>>> On 7/29/2022 5:48 AM, Si-Wei Liu wrote: >>>>> >>>>> >>>>> On 7/27/2022 7:43 PM, Zhu, Lingshan wrote: >>>>>> >>>>>> >>>>>> On 7/28/2022 8:56 AM, Si-Wei Liu wrote: >>>>>>> >>>>>>> >>>>>>> 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: >>>>>> that's why I proposed to fix these issues, e.g., if no _F_MAC, >>>>>> vDPA kernel should not return a mac to the userspace, there is >>>>>> not a default value for mac. >>>>> Then please show us the code, as I can only comment based on your >>>>> latest (v4) patch and it was not there.. To be honest, I don't >>>>> understand the motivation and the use cases you have, is it for >>>>> debugging/monitoring or there's really a use case for live >>>>> migration? For the former, you can do a direct dump on all config >>>>> space fields regardless of endianess and feature negotiation >>>>> without having to worry about validity (meaningful to present to >>>>> admin user). To me these are conflict asks that is impossible to >>>>> mix in exact one command. >>>> This bug just has been revealed two days, and you will see the >>>> patch soon. >>>> >>>> There are something to clarify: >>>> 1) we need to read the device features, or how can you pick a >>>> proper LM destination >> >> >> So it's probably not very efficient to use this, the manager layer >> should have the knowledge about the compatibility before doing >> migration other than try-and-fail. >> >> And it's the task of the management to gather the nodes whose devices >> could be live migrated to each other as something like "cluster" >> which we've already used in the case of cpuflags. >> >> 1) during node bootstrap, the capability of each node and devices was >> reported to management layer >> 2) management layer decide the cluster and make sure the migration >> can only done among the nodes insides the cluster >> 3) before migration, the vDPA needs to be provisioned on the destination >> >> >>>> 2) vdpa dev config show can show both device features and driver >>>> features, there just need a patch for iproute2 >>>> 3) To process information like MQ, we don't just dump the config >>>> space, MST has explained before >>> So, it's for live migration... Then why not export those config >>> parameters specified for vdpa creation (as well as device feature >>> bits) to the output of "vdpa dev show" command? That's where device >>> side config lives and is static across vdpa's life cycle. "vdpa dev >>> config show" is mostly for dynamic driver side config, and the >>> validity is subject to feature negotiation. I suppose this should >>> suit your need of LM, e.g. >> >> >> I think so. >> >> >>> >>> $ vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2 max_vqp 7 mtu 2000 >>> $ vdpa dev show vdpa1 >>> vdpa1: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs >>> 15 max_vq_size 256 >>> ? max_vqp 7 mtu 2000 >>> ? dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS >>> CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 RING_PACKED >> >> >> Note that the mgmt should know this destination have those >> capability/features before the provisioning. > Yes, mgmt software should have to check the above from source.On destination mgmt software can run below to check vdpa mgmtdev's capability/features: $ vdpa mgmtdev show pci/0000:41:04.3 pci/0000:41:04.3: ? supported_classes net ? max_supported_vqs 257 ? dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 RING_PACKED> >> >> >>> >>> For it to work, you'd want to pass "struct vdpa_dev_set_config" to >>> _vdpa_register_device() during registration, and get it saved there >>> in "struct vdpa_device". Then in vdpa_dev_fill() show each field >>> conditionally subject to "struct vdpa_dev_set_config.mask". >>> >>> Thanks, >>> -Siwei >> >> >> Thanks >> >> >>>> >>>> Thanks >>>> Zhu Lingshan >>>>> >>>>>>>> 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? >>>>>> They are driver_features(negotiated) and the >>>>>> device_features(which comes with the device), and the config >>>>>> space fields that depend on them. In this series, we report both >>>>>> to the userspace. >>>>> I fail to understand what you want to present from your >>>>> description. May be worth showing some example outputs that at >>>>> least include the following cases: 1) when device offers features >>>>> but not yet acknowledge by guest 2) when guest acknowledged >>>>> features and device is yet to accept 3) after guest feature >>>>> negotiation is completed (agreed upon between guest and device). >>>> Only two feature sets: 1) what the device has. (2) what is negotiated >>>>> >>>>> Thanks, >>>>> -Siwei >>>>>>> >>>>>>> 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://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NzOv5Ew_Z2CP-zHyD7RsUoStLZ54KpB21QyuZ8L63YVPLEGDEwvcOSDlIGxQPHY-DMkOa9sKKZdBSaNknMU$ >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Jason Wang
2022-Aug-02 06:33 UTC
[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space
On Tue, Aug 2, 2022 at 6:58 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote:> > > > On 8/1/2022 3:53 PM, Si-Wei Liu wrote: > > > > > > On 7/31/2022 9:44 PM, Jason Wang wrote: > >> > >> ? 2022/7/30 04:55, Si-Wei Liu ??: > >>> > >>> > >>> On 7/28/2022 7:04 PM, Zhu, Lingshan wrote: > >>>> > >>>> > >>>> On 7/29/2022 5:48 AM, Si-Wei Liu wrote: > >>>>> > >>>>> > >>>>> On 7/27/2022 7:43 PM, Zhu, Lingshan wrote: > >>>>>> > >>>>>> > >>>>>> On 7/28/2022 8:56 AM, Si-Wei Liu wrote: > >>>>>>> > >>>>>>> > >>>>>>> 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: > >>>>>> that's why I proposed to fix these issues, e.g., if no _F_MAC, > >>>>>> vDPA kernel should not return a mac to the userspace, there is > >>>>>> not a default value for mac. > >>>>> Then please show us the code, as I can only comment based on your > >>>>> latest (v4) patch and it was not there.. To be honest, I don't > >>>>> understand the motivation and the use cases you have, is it for > >>>>> debugging/monitoring or there's really a use case for live > >>>>> migration? For the former, you can do a direct dump on all config > >>>>> space fields regardless of endianess and feature negotiation > >>>>> without having to worry about validity (meaningful to present to > >>>>> admin user). To me these are conflict asks that is impossible to > >>>>> mix in exact one command. > >>>> This bug just has been revealed two days, and you will see the > >>>> patch soon. > >>>> > >>>> There are something to clarify: > >>>> 1) we need to read the device features, or how can you pick a > >>>> proper LM destination > >> > >> > >> So it's probably not very efficient to use this, the manager layer > >> should have the knowledge about the compatibility before doing > >> migration other than try-and-fail. > >> > >> And it's the task of the management to gather the nodes whose devices > >> could be live migrated to each other as something like "cluster" > >> which we've already used in the case of cpuflags. > >> > >> 1) during node bootstrap, the capability of each node and devices was > >> reported to management layer > >> 2) management layer decide the cluster and make sure the migration > >> can only done among the nodes insides the cluster > >> 3) before migration, the vDPA needs to be provisioned on the destination > >> > >> > >>>> 2) vdpa dev config show can show both device features and driver > >>>> features, there just need a patch for iproute2 > >>>> 3) To process information like MQ, we don't just dump the config > >>>> space, MST has explained before > >>> So, it's for live migration... Then why not export those config > >>> parameters specified for vdpa creation (as well as device feature > >>> bits) to the output of "vdpa dev show" command? That's where device > >>> side config lives and is static across vdpa's life cycle. "vdpa dev > >>> config show" is mostly for dynamic driver side config, and the > >>> validity is subject to feature negotiation. I suppose this should > >>> suit your need of LM, e.g. > >> > >> > >> I think so. > >> > >> > >>> > >>> $ vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2 max_vqp 7 mtu 2000 > >>> $ vdpa dev show vdpa1 > >>> vdpa1: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs > >>> 15 max_vq_size 256 > >>> max_vqp 7 mtu 2000 > >>> dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS > >>> CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 RING_PACKED > >> > >> > >> Note that the mgmt should know this destination have those > >> capability/features before the provisioning. > > Yes, mgmt software should have to check the above from source. > > On destination mgmt software can run below to check vdpa mgmtdev's > capability/features: > > $ vdpa mgmtdev show pci/0000:41:04.3 > pci/0000:41:04.3: > supported_classes net > max_supported_vqs 257 > dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ > MQ CTRL_MAC_ADDR VERSION_1 RING_PACKEDRight and this is probably better to be done at node bootstrapping for the management to know about the cluster. Thanks> > > >> > >> > >>> > >>> For it to work, you'd want to pass "struct vdpa_dev_set_config" to > >>> _vdpa_register_device() during registration, and get it saved there > >>> in "struct vdpa_device". Then in vdpa_dev_fill() show each field > >>> conditionally subject to "struct vdpa_dev_set_config.mask". > >>> > >>> Thanks, > >>> -Siwei > >> > >> > >> Thanks > >> > >> > >>>> > >>>> Thanks > >>>> Zhu Lingshan > >>>>> > >>>>>>>> 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? > >>>>>> They are driver_features(negotiated) and the > >>>>>> device_features(which comes with the device), and the config > >>>>>> space fields that depend on them. In this series, we report both > >>>>>> to the userspace. > >>>>> I fail to understand what you want to present from your > >>>>> description. May be worth showing some example outputs that at > >>>>> least include the following cases: 1) when device offers features > >>>>> but not yet acknowledge by guest 2) when guest acknowledged > >>>>> features and device is yet to accept 3) after guest feature > >>>>> negotiation is completed (agreed upon between guest and device). > >>>> Only two feature sets: 1) what the device has. (2) what is negotiated > >>>>> > >>>>> Thanks, > >>>>> -Siwei > >>>>>>> > >>>>>>> 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://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NzOv5Ew_Z2CP-zHyD7RsUoStLZ54KpB21QyuZ8L63YVPLEGDEwvcOSDlIGxQPHY-DMkOa9sKKZdBSaNknMU$ > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >