| Message ID | 20210104033141.105876-8-parav@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce vdpa management tool | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2021/1/4 上午11:31, Parav Pandit wrote: > static int __init vdpasim_net_init(void) > { > int ret = 0; > @@ -176,6 +264,8 @@ static int __init vdpasim_net_init(void) > > if (default_device) > ret = vdpasim_net_default_dev_register(); > + else > + ret = vdpasim_net_mgmtdev_init(); > return ret; > } > > @@ -183,6 +273,8 @@ static void __exit vdpasim_net_exit(void) > { > if (default_device) > vdpasim_net_default_dev_unregister(); > + else > + vdpasim_net_mgmtdev_cleanup(); > } > > module_init(vdpasim_net_init); > -- 2.26.2 I wonder what's the value of keeping the default device that is out of the control of management API. Thanks
> From: Jason Wang <jasowang@redhat.com> > Sent: Monday, January 4, 2021 12:35 PM > > On 2021/1/4 上午11:31, Parav Pandit wrote: > > static int __init vdpasim_net_init(void) > > { > > int ret = 0; > > @@ -176,6 +264,8 @@ static int __init vdpasim_net_init(void) > > > > if (default_device) > > ret = vdpasim_net_default_dev_register(); > > + else > > + ret = vdpasim_net_mgmtdev_init(); > > return ret; > > } > > > > @@ -183,6 +273,8 @@ static void __exit vdpasim_net_exit(void) > > { > > if (default_device) > > vdpasim_net_default_dev_unregister(); > > + else > > + vdpasim_net_mgmtdev_cleanup(); > > } > > > > module_init(vdpasim_net_init); > > -- 2.26.2 > > > I wonder what's the value of keeping the default device that is out of the > control of management API. I think we can remove it like how I did in the v1 version. And actual vendor drivers like mlx5_vdpa will likely should do only user created devices. I added only for backward compatibility purpose, but we can remove the default simulated vdpa net device. What do you recommend?
On 2021/1/4 下午3:21, Parav Pandit wrote: > >> From: Jason Wang <jasowang@redhat.com> >> Sent: Monday, January 4, 2021 12:35 PM >> >> On 2021/1/4 上午11:31, Parav Pandit wrote: >>> static int __init vdpasim_net_init(void) >>> { >>> int ret = 0; >>> @@ -176,6 +264,8 @@ static int __init vdpasim_net_init(void) >>> >>> if (default_device) >>> ret = vdpasim_net_default_dev_register(); >>> + else >>> + ret = vdpasim_net_mgmtdev_init(); >>> return ret; >>> } >>> >>> @@ -183,6 +273,8 @@ static void __exit vdpasim_net_exit(void) >>> { >>> if (default_device) >>> vdpasim_net_default_dev_unregister(); >>> + else >>> + vdpasim_net_mgmtdev_cleanup(); >>> } >>> >>> module_init(vdpasim_net_init); >>> -- 2.26.2 >> >> I wonder what's the value of keeping the default device that is out of the >> control of management API. > I think we can remove it like how I did in the v1 version. And actual vendor drivers like mlx5_vdpa will likely should do only user created devices. > I added only for backward compatibility purpose, but we can remove the default simulated vdpa net device. > What do you recommend? I think we'd better mandate this management API. This can avoid vendor specific configuration that may complex management layer. Thanks
> From: Jason Wang <jasowang@redhat.com> > Sent: Tuesday, January 5, 2021 9:36 AM > > > On 2021/1/4 下午3:21, Parav Pandit wrote: > > > >> From: Jason Wang <jasowang@redhat.com> > >> Sent: Monday, January 4, 2021 12:35 PM > >> > >> On 2021/1/4 上午11:31, Parav Pandit wrote: > >>> static int __init vdpasim_net_init(void) > >>> { > >>> int ret = 0; > >>> @@ -176,6 +264,8 @@ static int __init vdpasim_net_init(void) > >>> > >>> if (default_device) > >>> ret = vdpasim_net_default_dev_register(); > >>> + else > >>> + ret = vdpasim_net_mgmtdev_init(); > >>> return ret; > >>> } > >>> > >>> @@ -183,6 +273,8 @@ static void __exit vdpasim_net_exit(void) > >>> { > >>> if (default_device) > >>> vdpasim_net_default_dev_unregister(); > >>> + else > >>> + vdpasim_net_mgmtdev_cleanup(); > >>> } > >>> > >>> module_init(vdpasim_net_init); > >>> -- 2.26.2 > >> > >> I wonder what's the value of keeping the default device that is out > >> of the control of management API. > > I think we can remove it like how I did in the v1 version. And actual vendor > drivers like mlx5_vdpa will likely should do only user created devices. > > I added only for backward compatibility purpose, but we can remove the > default simulated vdpa net device. > > What do you recommend? > > > I think we'd better mandate this management API. This can avoid vendor > specific configuration that may complex management layer. > Sounds good. I will drop the patch that allows vdpasim_net default device via module parameter. Will post v3 with that removal.
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index db1636a99ba4..d5942842432d 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -235,7 +235,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) ops = &vdpasim_config_ops; vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops, - dev_attr->nvqs, NULL); + dev_attr->nvqs, dev_attr->name); if (!vdpasim) goto err_alloc; @@ -249,6 +249,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64))) goto err_iommu; set_dma_ops(dev, &vdpasim_dma_ops); + vdpasim->vdpa.mdev = dev_attr->mgmt_dev; vdpasim->config = kzalloc(dev_attr->config_size, GFP_KERNEL); if (!vdpasim->config) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h index b02142293d5b..6d75444f9948 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h @@ -33,6 +33,8 @@ struct vdpasim_virtqueue { }; struct vdpasim_dev_attr { + struct vdpa_mgmt_dev *mgmt_dev; + const char *name; u64 supported_features; size_t config_size; size_t buffer_size; diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c index 34155831538c..b795e02bdad0 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c @@ -162,6 +162,94 @@ static void vdpasim_net_default_dev_unregister(void) vdpa_unregister_device(vdpa); } +static void vdpasim_net_mgmtdev_release(struct device *dev) +{ +} + +static struct device vdpasim_net_mgmtdev = { + .init_name = "vdpasim_net", + .release = vdpasim_net_mgmtdev_release, +}; + +static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) +{ + struct vdpasim_dev_attr dev_attr = {}; + struct vdpasim *simdev; + int ret; + + dev_attr.mgmt_dev = mdev; + dev_attr.name = name; + dev_attr.id = VIRTIO_ID_NET; + dev_attr.supported_features = VDPASIM_NET_FEATURES; + dev_attr.nvqs = VDPASIM_NET_VQ_NUM; + dev_attr.config_size = sizeof(struct virtio_net_config); + dev_attr.get_config = vdpasim_net_get_config; + dev_attr.work_fn = vdpasim_net_work; + dev_attr.buffer_size = PAGE_SIZE; + + simdev = vdpasim_create(&dev_attr); + if (IS_ERR(simdev)) + return PTR_ERR(simdev); + + ret = _vdpa_register_device(&simdev->vdpa); + if (ret) + goto reg_err; + + return 0; + +reg_err: + put_device(&simdev->vdpa.dev); + return ret; +} + +static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, + struct vdpa_device *dev) +{ + struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); + + _vdpa_unregister_device(&simdev->vdpa); +} + +static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { + .dev_add = vdpasim_net_dev_add, + .dev_del = vdpasim_net_dev_del +}; + +static struct virtio_device_id id_table[] = { + { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID }, + { 0 }, +}; + +static struct vdpa_mgmt_dev mgmt_dev = { + .device = &vdpasim_net_mgmtdev, + .id_table = id_table, + .ops = &vdpasim_net_mgmtdev_ops, +}; + +static int vdpasim_net_mgmtdev_init(void) +{ + int ret; + + ret = device_register(&vdpasim_net_mgmtdev); + if (ret) + return ret; + + ret = vdpa_mgmtdev_register(&mgmt_dev); + if (ret) + goto parent_err; + return 0; + +parent_err: + device_unregister(&vdpasim_net_mgmtdev); + return ret; +} + +static void vdpasim_net_mgmtdev_cleanup(void) +{ + vdpa_mgmtdev_unregister(&mgmt_dev); + device_unregister(&vdpasim_net_mgmtdev); +} + static int __init vdpasim_net_init(void) { int ret = 0; @@ -176,6 +264,8 @@ static int __init vdpasim_net_init(void) if (default_device) ret = vdpasim_net_default_dev_register(); + else + ret = vdpasim_net_mgmtdev_init(); return ret; } @@ -183,6 +273,8 @@ static void __exit vdpasim_net_exit(void) { if (default_device) vdpasim_net_default_dev_unregister(); + else + vdpasim_net_mgmtdev_cleanup(); } module_init(vdpasim_net_init);
Enable user to create vdpasim net simulate devices. Show vdpa management device that supports creating, deleting vdpa devices. $ vdpa mgmtdev show vdpasim_net: supported_classes net $ vdpa mgmtdev show -jp { "show": { "vdpasim_net": { "supported_classes": { "net" } } } Create a vdpa device of type networking named as "foo2" from the management device vdpasim: $ vdpa dev add mgmtdev vdpasim_net name foo2 Show the newly created vdpa device by its name: $ vdpa dev show foo2 foo2: type network mgmtdev vdpasim_net vendor_id 0 max_vqs 2 max_vq_size 256 $ vdpa dev show foo2 -jp { "dev": { "foo2": { "type": "network", "mgmtdev": "vdpasim_net", "vendor_id": 0, "max_vqs": 2, "max_vq_size": 256 } } } Delete the vdpa device after its use: $ vdpa dev del foo2 Signed-off-by: Parav Pandit <parav@nvidia.com> Reviewed-by: Eli Cohen <elic@nvidia.com> Acked-by: Jason Wang <jasowang@redhat.com> --- Changelog: v1->v2: - rebased --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +- drivers/vdpa/vdpa_sim/vdpa_sim.h | 2 + drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 92 ++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-)