[v2,3/4] vdpa: show dev config as-is in "vdpa dev show" output

  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 from
which vdpa device was created as-is. The "vdpa dev show" command
seems to be the right vehicle for that. It is unlike the "vdpa dev
config show" command output that usually goes with the live value
in the device config space, which is not quite reliable subject to
the dynamics of feature negotiation and possible change in device
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
  virtio_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,
            "virtio_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(+)
  

On Wed, Oct 19, 2022 at 8:56 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 from
> which vdpa device was created as-is. The "vdpa dev show" command
> seems to be the right vehicle for that. It is unlike the "vdpa dev
> config show" command output that usually goes with the live value
> in the device config space, which is not quite reliable subject to
> the dynamics of feature negotiation and possible change in device
> 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
>   virtio_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,
>             "virtio_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 566c1c6..91eca6d 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_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
> +{
> +       struct vdpa_dev_set_config *cfg = &vdev->vdev_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;

This makes me think if we can reuse the virtio_net_config structure
other than duplicate it slowly with a dedicated nested structure
inside vdpa_dev_set_config then we can reuse the
vdpa_dev_net_config_fill().

Thanks

> +       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;
> +
> +       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_cfgattrs_fill(vdev, msg, device_id);
> +       if (err)
> +               goto msg_err;
> +
>         genlmsg_end(msg, hdr);
>         return 0;
>
> --
> 1.8.3.1
>
  

On 10/19/2022 10:25 PM, Jason Wang wrote:
> On Wed, Oct 19, 2022 at 8:56 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 from
>> which vdpa device was created as-is. The "vdpa dev show" command
>> seems to be the right vehicle for that. It is unlike the "vdpa dev
>> config show" command output that usually goes with the live value
>> in the device config space, which is not quite reliable subject to
>> the dynamics of feature negotiation and possible change in device
>> 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
>>    virtio_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,
>>              "virtio_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 566c1c6..91eca6d 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_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id)
>> +{
>> +       struct vdpa_dev_set_config *cfg = &vdev->vdev_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;
> This makes me think if we can reuse the virtio_net_config structure
> other than duplicate it slowly with a dedicated nested structure
> inside vdpa_dev_set_config then we can reuse the
> vdpa_dev_net_config_fill().
Adding Parav.

I think for now the struct vdpa_dev_set_config has just a few fields, so 
it's not very obvious. But from what I understand, the 
vdpa_dev_set_config struct is designed to be built around vdpa 
configurables, without getting it limited by what's exposed by the 
virtio device config structure, such as virtio_net_config. For instance, 
there could be possibility for vdpa user to specify the size of MAC 
unicast or multicast address table, which is not defined anywhere in the 
virtio_net_config. I think it's important to match such configuration 
(which may not even be defined in spec) for src&dst vdpa devices 
involving the live migration.

-Siwei
>
> Thanks
>
>> +       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;
>> +
>> +       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_cfgattrs_fill(vdev, msg, device_id);
>> +       if (err)
>> +               goto msg_err;
>> +
>>          genlmsg_end(msg, hdr);
>>          return 0;
>>
>> --
>> 1.8.3.1
>>
  

> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Thursday, October 20, 2022 2:12 PM
> 
> 
> On 10/19/2022 10:25 PM, Jason Wang wrote:
> > On Wed, Oct 19, 2022 at 8:56 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 from which vdpa device was created
> >> as-is. The "vdpa dev show" command seems to be the right vehicle for
> >> that. It is unlike the "vdpa dev config show" command output that
> >> usually goes with the live value in the device config space, which is
> >> not quite reliable subject to the dynamics of feature negotiation and
> >> possible change in device 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
> >>    virtio_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,
> >>              "virtio_config": {
Since most config is related to virtio.
May be better to do
s/virtio_config/static_config or
s/virto_config/dev_config

This clearly indicates that this was the device static configuration.

> >>                  "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
> >> 566c1c6..91eca6d 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_cfgattrs_fill(struct vdpa_device *vdev, struct sk_buff
> >> +*msg, u32 device_id) {
> >> +       struct vdpa_dev_set_config *cfg = &vdev->vdev_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;
> > This makes me think if we can reuse the virtio_net_config structure
> > other than duplicate it slowly with a dedicated nested structure
> > inside vdpa_dev_set_config then we can reuse the
> > vdpa_dev_net_config_fill().
> Adding Parav.
> 
> I think for now the struct vdpa_dev_set_config has just a few fields, so it's
> not very obvious. But from what I understand, the vdpa_dev_set_config
> struct is designed to be built around vdpa configurables, without getting it
> limited by what's exposed by the virtio device config structure, such as
> virtio_net_config. 
Sure. Vdpa_dev_set_config can expand for fields outside of virtio_net_config structure space, but it should be close to virtio spec definition like you described below or close to Linux kernel objects.

At present it can handle another 62 more fields, which I think is good enough for midterm.

> For instance, there could be possibility for vdpa user to
> specify the size of MAC unicast or multicast address table, which is not
> defined anywhere in the virtio_net_config. I think it's important to match
> such configuration (which may not even be defined in spec) for src&dst vdpa
> devices involving the live migration.
> 
> -Siwei
> >
> > Thanks
> >
> >> +       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;
> >> +
> >> +       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_cfgattrs_fill(vdev, msg, device_id);
> >> +       if (err)
> >> +               goto msg_err;
> >> +
> >>          genlmsg_end(msg, hdr);
> >>          return 0;
> >>
> >> --
> >> 1.8.3.1
> >>