thr3ads.net - Virtualization - [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space [Jul 2022]

If this information is useful, please help other people find it:
Share via:

Parav Pandit

2022-Jul-01 22:12 UTC

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

> 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

Michael S. Tsirkin

2022-Jul-13 05:23 UTC

head link

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

On Fri, Jul 01, 2022 at 10:12:49PM +0000, Parav Pandit
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
arec> > 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")
Yea you normally use

--format='Fixes: %h (\"%s\")'

> 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.

I don't think checkpatch complains about this if for no other reason
that sometimes the 6 byte hash is not enough.
> > 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

Si-Wei Liu

2022-Jul-27 09:43 UTC

head link

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

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.

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.

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 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/14bf5b1e/attachment-0001.html>

Virtualization - Jul 2022 - [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space