Date:   Thu, 28 Jul 2022 10:06:12 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Si-Wei Liu <si-wei.liu@...cle.com>,
        "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        Parav Pandit <parav@...dia.com>,
        "mst@...hat.com" <mst@...hat.com>, Eli Cohen <elic@...dia.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [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@...el.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@...el.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@...ts.linux-foundation.org
>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>
>>
>

Powered by blists - more mailing lists