Re: [PATCH v3 3/4] vdpa: show dev config as-is in "vdpa dev show" output

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

[-- Attachment #1.1: Type: text/plain, Size: 8352 bytes --]



On 10/24/2022 7:24 PM, Jason Wang wrote:
> On Tue, Oct 25, 2022 at 3:14 AM Si-Wei Liu<si-wei.liu@oracle.com>  wrote:
>>
>>
>> On 10/24/2022 1:40 AM, Jason Wang wrote:
>>> On Sat, Oct 22, 2022 at 7:49 AM Si-Wei Liu<si-wei.liu@oracle.com>  wrote:
>>>> Live migration of vdpa would typically require re-instate vdpa
>>>> device with an idential set of configs on the destination node,
>>>> same way as how source node created the device in the first
>>>> place. In order to save orchestration software from memorizing
>>>> and keeping track of vdpa config, it will be helpful if the vdpa
>>>> tool provides the aids for exporting the initial configs as-is,
>>>> the way how vdpa device was created. The "vdpa dev show" command
>>>> seems to be the right vehicle for that. It is unlike the "vdpa dev
>>>> config show" command output which usually goes with the live value
>>>> in the device config space, and is not quite reliable subject to
>>>> the dynamics of feature negotiation or possible change by the
>>>> driver to the config space.
>>>>
>>>> Examples:
>>>>
>>>> 1) Create vDPA by default without any config attribute
>>>>
>>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0
>>>> $ vdpa dev show vdpa0
>>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
>>>> $ vdpa dev -jp show vdpa0
>>>> {
>>>>       "dev": {
>>>>           "vdpa0": {
>>>>               "type": "network",
>>>>               "mgmtdev": "pci/0000:41:04.2",
>>>>               "vendor_id": 5555,
>>>>               "max_vqs": 9,
>>>>               "max_vq_size": 256,
>>>>           }
>>>>       }
>>>> }
>>>>
>>>> 2) Create vDPA with config attribute(s) specified
>>>>
>>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \
>>>>       mac e4:11:c6:d3:45:f0 max_vq_pairs 4
>>>> $ vdpa dev show
>>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256
>>>>     initial_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4
>>>> $ vdpa dev -jp show
>>>> {
>>>>       "dev": {
>>>>           "vdpa0": {
>>>>               "type": "network",
>>>>               "mgmtdev": "pci/0000:41:04.2",
>>>>               "vendor_id": 5555,
>>>>               "max_vqs": 9,
>>>>               "max_vq_size": 256,
>>>>               "initial_config": {
>>>>                   "mac": "e4:11:c6:d3:45:f0",
>>>>                   "max_vq_pairs": 4
>>>>               }
>>>>           }
>>>>       }
>>>> }
>>>>
>>>> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index bebded6..bfb8f54 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>>>>    }
>>>>
>>>>    static int
>>>> +vdpa_dev_initcfg_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
>>>> +{
>>>> +       struct vdpa_dev_set_config *cfg = &vdev->init_cfg;
>>>> +       int err = -EMSGSIZE;
>>>> +
>>>> +       if (!cfg->mask)
>>>> +               return 0;
>>>> +
>>>> +       switch (device_id) {
>>>> +       case VIRTIO_ID_NET:
>>>> +               if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 &&
>>>> +                   nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
>>>> +                           sizeof(cfg->net.mac), cfg->net.mac))
>>>> +                       return err;
>>>> +               if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 &&
>>>> +                   nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu))
>>>> +                       return err;
>>>> +               if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 &&
>>>> +                   nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>> +                               cfg->net.max_vq_pairs))
>>>> +                       return err;
>>>> +               break;
>>>> +       default:
>>>> +               break;
>>>> +       }
>>>> +
>>>> +       if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 &&
>>>> +           nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
>>>> +                             cfg->device_features, VDPA_ATTR_PAD))
>>>> +               return err;
>>> A question: If any of those above attributes were not provisioned,
>>> should we show the ones that are inherited from the parent?
>> A simple answer would be yes, but the long answer is that I am not sure
>> if there's any for the moment - there's no  default value for mtu, mac,
>> and max_vqp that can be inherited from the parent (max_vqp by default
>> being 1 is spec defined, not something inherited from the parent).
> Note that it is by default from driver level that if _F_MQ is not
> negotiated. But I think we are talking about something different that
> is out of the spec here, what if:
>
> vDPA inherit _F_MQ but mqx_vqp is not provisioned via netlink.
>
> Or is it not allowed?
My understanding is that this is not allowed any more since the 
introduction of max_vqp attribute. Noted, currently we don't have a way 
for vendor driver to report the default value for mqx_vqp, if not 
otherwise specified in the CLI. Without getting the default value 
reported in 'vdpa mgmtdev show' level, it'd just confuse mgmt software 
even more.

>    At least some time in the past, mlx5 were
> enabled with MQ with 8 queue pairs by default.
That was the situation when there's no max_vqp attribute support from 
vdpa netlink API level. I think now every driver honors the vdpa core 
disposition to get a single queue pair if max_vqp config is missing.  
And the mlx5_vdpa driver with 8 queue pairs in the wild days is just 
irrelevant to be manageable by mgmt software, regardless of live migration.
>
>> And
>> the device_features if inherited is displayed at 'vdpa dev config show'
>> output. Can you remind me of a good example for inherited value that we
>> may want to show here?
> Some other cases:
>
> 1) MTU: there should be something that the device needs to report if
> _F_MTU is negotiated even if it is not provisioned from netlink.
I am not sure I understand the ask here. Noted the QEMU argument has to 
offer host_mtu=X with the maximum MTU value for guest to use (and 
applied as the initial MTU config during virtio-net probing for Linux 
driver), and the way to get the parent device MTU and whether that's 
relevant to vdpa device's MTU is very vendor specific. I think we would 
need new attribute(s) in the mgmtdev level to support what you want here?

> 2) device_features: if device_features is not provisioned, we should
> still report it via netlink here
Not the way I expected it, but with Lingshan's series to expose fields 
out of FEATURES_OK, the device_features is now reported through 'vdpa 
dev config show' regardless being specified or not, if I am not mistaken?

Currently we export the config attributes upon vdpa creation under the 
"initial_config" key. If we want to expose more default values inherited 
from mgmtdev, I think we can wrap up these default values under another 
key "inherited_config" to display in 'vdpa dev show' output. Does it fit 
what you have in mind?

> or do you mean the mgmt can assume it
> should be the same as mgmtdev. Anyhow if we don't show device_features
> if it is not provisioned, it will complicate the mgmt software.
Yes, as I said earlier, since the device_features attr getting added to 
the 'vdpa dev config show' command, this divergence started to 
complicate mgmt software already.

Thanks,
-Siwei
>
> Thanks
>
>>
>> Thanks,
>> -Siwei
>>
>>
>>> Thanks
>>>
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int
>>>>    vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
>>>>                 int flags, struct netlink_ext_ack *extack)
>>>>    {
>>>> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>>>>           if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size))
>>>>                   goto msg_err;
>>>>
>>>> +       err = vdpa_dev_initcfg_fill(vdev, msg, device_id);
>>>> +       if (err)
>>>> +               goto msg_err;
>>>> +
>>>>           genlmsg_end(msg, hdr);
>>>>           return 0;
>>>>
>>>> --
>>>> 1.8.3.1
>>>>

[-- Attachment #1.2: Type: text/html, Size: 10664 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization