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

[Si-Wei Liu]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   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