Jason Wang
2022-Oct-20 05:20 UTC
[PATCH v2 2/4] vdpa: pass initial config to _vdpa_register_device()
On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote:> > Just as _vdpa_register_device taking @nvqs as the number of queuesI wonder if it's better to embed nvqs in the config structure.> to feed userspace inquery via vdpa_dev_fill(), we can follow the > same to stash config attributes in struct vdpa_device at the time > of vdpa registration. > > Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- > drivers/vdpa/vdpa.c | 15 +++++++++++---- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +- > drivers/vdpa/vdpa_user/vduse_dev.c | 2 +- > drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++- > include/linux/vdpa.h | 3 ++- > 8 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index f9c0044..c54ab2c 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > else > ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index); > > - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring); > + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config); > if (ret) { > put_device(&adapter->vdpa.dev); > IFCVF_ERR(pdev, "Failed to register to vDPA bus"); > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 9091336..376082e 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > mlx5_notifier_register(mdev, &ndev->nb); > ndev->nb_registered = true; > mvdev->vdev.mdev = &mgtdev->mgtdev; > - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1); > + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config); > if (err) > goto err_reg; > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index febdc99..566c1c6 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data) > return (strcmp(dev_name(&vdev->dev), data) == 0); > } > > -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs) > +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs, > + const struct vdpa_dev_set_config *cfg) > { > struct device *dev; > > vdev->nvqs = nvqs; > + if (cfg) > + vdev->vdev_cfg = *cfg; > + else > + vdev->vdev_cfg.mask = 0ULL;I think it would be nice if we can convert eni to use netlink then we don't need any workaround like this. Thanks
Si-Wei Liu
2022-Oct-20 17:42 UTC
[PATCH v2 2/4] vdpa: pass initial config to _vdpa_register_device()
On 10/19/2022 10:20 PM, Jason Wang wrote:> On Wed, Oct 19, 2022 at 8:56 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote: >> Just as _vdpa_register_device taking @nvqs as the number of queues > I wonder if it's better to embed nvqs in the config structure.Hmmm, the config structure is mostly for containing the configurables specified in the 'vdpa dev add' command, while each field is conditionally set and guarded by a corresponding mask bit. If @nvqs needs to be folded into a structure, I feel it might be better to use another struct for holding the informational fields (i.e. those are read-only and always exist). But doing this would make @nvqs a weird solo member in that struct with no extra benefit, and all the other informational fields shown in the 'vdpa dev show' command would be gotten from the device through config_ops directly. Maybe do this until another read-only field comes around?> >> to feed userspace inquery via vdpa_dev_fill(), we can follow the >> same to stash config attributes in struct vdpa_device at the time >> of vdpa registration. >> >> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- >> drivers/vdpa/vdpa.c | 15 +++++++++++---- >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 +- >> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 +- >> drivers/vdpa/vdpa_user/vduse_dev.c | 2 +- >> drivers/vdpa/virtio_pci/vp_vdpa.c | 3 ++- >> include/linux/vdpa.h | 3 ++- >> 8 files changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >> index f9c0044..c54ab2c 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -771,7 +771,7 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, >> else >> ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index); >> >> - ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring); >> + ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring, config); >> if (ret) { >> put_device(&adapter->vdpa.dev); >> IFCVF_ERR(pdev, "Failed to register to vDPA bus"); >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> index 9091336..376082e 100644 >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> @@ -3206,7 +3206,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, >> mlx5_notifier_register(mdev, &ndev->nb); >> ndev->nb_registered = true; >> mvdev->vdev.mdev = &mgtdev->mgtdev; >> - err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1); >> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1, add_config); >> if (err) >> goto err_reg; >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index febdc99..566c1c6 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -215,11 +215,16 @@ static int vdpa_name_match(struct device *dev, const void *data) >> return (strcmp(dev_name(&vdev->dev), data) == 0); >> } >> >> -static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs) >> +static int __vdpa_register_device(struct vdpa_device *vdev, u32 nvqs, >> + const struct vdpa_dev_set_config *cfg) >> { >> struct device *dev; >> >> vdev->nvqs = nvqs; >> + if (cfg) >> + vdev->vdev_cfg = *cfg; >> + else >> + vdev->vdev_cfg.mask = 0ULL; > I think it would be nice if we can convert eni to use netlink then we > don't need any workaround like this.Yes, Alibaba ENI is the only consumer of the old vdpa_register_device() API without being ported to the netlink API. Not sure what is needed but it seems another work to make netlink API committed to support a legacy compatible model? -Siwei> > Thanks >