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

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 8/1/2022 11:33 PM, Jason Wang wrote:
> On Tue, Aug 2, 2022 at 6:58 AM Si-Wei Liu <si-wei.liu@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_PACKED
> Right and this is probably better to be done at node bootstrapping for
> the management to know about the cluster.
Exactly. That's what mgmt software is supposed to do typically.

Thanks,
-Siwei

>
> 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@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@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@lists.linux-foundation.org
>>>>>>>>>>>> https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NzOv5Ew_Z2CP-zHyD7RsUoStLZ54KpB21QyuZ8L63YVPLEGDEwvcOSDlIGxQPHY-DMkOa9sKKZdBSaNknMU$
>>>>>>>>>>>
>>>>>>>>>>>