Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules
On 2020/11/18 下午9:14, Stefano Garzarella wrote:
Hi Jason,
I just discovered that I missed the other questions in this email,
sorry for that!
No problem :)
On Mon, Nov 16, 2020 at 12:00:11PM +0800, Jason Wang wrote:
On 2020/11/13 下午9:47, Stefano Garzarella wrote:
From: Max Gurtovoy
Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a
preparation for adding a vdpa simulator module for block devices.
Signed-off-by: Max Gurtovoy
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella
---
v1:
- Removed unused headers
- Removed empty module_init() module_exit()
- Moved vdpasim_is_little_endian() in vdpa_sim.h
- Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h
- Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
option can not depend on other [Jason]
If possible, I would suggest to split this patch further:
1) convert to use void *config, and an attribute for setting config
size during allocation
2) introduce supported_features
3) other attributes (#vqs)
4) rename config ops (more generic one)
5) introduce ops for set|get_config, set_get_features
6) real split
[...]
-static const struct vdpa_config_ops vdpasim_net_config_ops;
-static const struct vdpa_config_ops vdpasim_net_batch_config_ops;
+static const struct vdpa_config_ops vdpasim_config_ops;
+static const struct vdpa_config_ops vdpasim_batch_config_ops;
-static struct vdpasim *vdpasim_create(void)
+struct vdpasim *vdpasim_create(struct vdpasim_init_attr *attr)
{
const struct vdpa_config_ops *ops;
struct vdpasim *vdpasim;
+ u32 device_id;
struct device *dev;
- int ret = -ENOMEM;
+ int i, size, ret = -ENOMEM;
- if (batch_mapping)
- ops = _net_batch_config_ops;
+ device_id = attr->device_id;
+ /* Currently, we only accept the network and block devices. */
+ if (device_id != VIRTIO_ID_NET && device_id != VIRTIO_ID_BLOCK)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ if (attr->batch_mapping)
+ ops = _batch_config_ops;
else
- ops = _net_config_ops;
+ ops = _config_ops;
vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops,
VDPASIM_VQ_NUM);
if (!vdpasim)
goto err_alloc;
- INIT_WORK(>work, vdpasim_work);
+ if (device_id == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+ else
+ size = sizeof(struct virtio_blk_config);
It's better to avoid such if/else consider we may introduce more type
of devices.
Can we have an attribute of config size instead?
Yes, I'll move the patch 7 before this.
About config size and set/get_config ops, I'm not sure if it is better
to hidden everything under the new set/get_config ops, allocating the
config structure in each device, or leave the allocation in the core
and update it like now.
I think we'd better to avoid having any type specific codes in generic
sim codes.
[...]
+config VDPA_SIM_NET
+ tristate "vDPA simulator for networking device"
+ depends on VDPA_SIM
+ default n
I remember somebody told me that if we don't enable a module it was
disabled by default.
So, should I remove "default n" from vdpa_sim* entries?
Yes, but please do that in another patch.
Thanks
Thanks,
Stefano
Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules
Hi Jason,
I just discovered that I missed the other questions in this email,
sorry for that!
On Mon, Nov 16, 2020 at 12:00:11PM +0800, Jason Wang wrote:
On 2020/11/13 下午9:47, Stefano Garzarella wrote:
From: Max Gurtovoy
Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a
preparation for adding a vdpa simulator module for block devices.
Signed-off-by: Max Gurtovoy
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella
---
v1:
- Removed unused headers
- Removed empty module_init() module_exit()
- Moved vdpasim_is_little_endian() in vdpa_sim.h
- Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h
- Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
option can not depend on other [Jason]
If possible, I would suggest to split this patch further:
1) convert to use void *config, and an attribute for setting config
size during allocation
2) introduce supported_features
3) other attributes (#vqs)
4) rename config ops (more generic one)
5) introduce ops for set|get_config, set_get_features
6) real split
[...]
-static const struct vdpa_config_ops vdpasim_net_config_ops;
-static const struct vdpa_config_ops vdpasim_net_batch_config_ops;
+static const struct vdpa_config_ops vdpasim_config_ops;
+static const struct vdpa_config_ops vdpasim_batch_config_ops;
-static struct vdpasim *vdpasim_create(void)
+struct vdpasim *vdpasim_create(struct vdpasim_init_attr *attr)
{
const struct vdpa_config_ops *ops;
struct vdpasim *vdpasim;
+ u32 device_id;
struct device *dev;
- int ret = -ENOMEM;
+ int i, size, ret = -ENOMEM;
- if (batch_mapping)
- ops = _net_batch_config_ops;
+ device_id = attr->device_id;
+ /* Currently, we only accept the network and block devices. */
+ if (device_id != VIRTIO_ID_NET && device_id != VIRTIO_ID_BLOCK)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ if (attr->batch_mapping)
+ ops = _batch_config_ops;
else
- ops = _net_config_ops;
+ ops = _config_ops;
vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops,
VDPASIM_VQ_NUM);
if (!vdpasim)
goto err_alloc;
- INIT_WORK(>work, vdpasim_work);
+ if (device_id == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+ else
+ size = sizeof(struct virtio_blk_config);
It's better to avoid such if/else consider we may introduce more type
of devices.
Can we have an attribute of config size instead?
Yes, I'll move the patch 7 before this.
About config size and set/get_config ops, I'm not sure if it is better
to hidden everything under the new set/get_config ops, allocating the
config structure in each device, or leave the allocation in the core and
update it like now.
+ vdpasim->config = kzalloc(size, GFP_KERNEL);
+ if (!vdpasim->config)
+ goto err_iommu;
+
+ vdpasim->device_id = device_id;
+ vdpasim->supported_features = attr->features;
+ INIT_WORK(>work, attr->work_fn);
spin_lock_init(>lock);
spin_lock_init(>iommu_lock);
@@ -379,23 +231,10 @@ static struct vdpasim *vdpasim_create(void)
if (!vdpasim->buffer)
goto err_iommu;
- if (macaddr) {
- mac_pton(macaddr, vdpasim->config.mac);
- if (!is_valid_ether_addr(vdpasim->config.mac)) {
- ret = -EADDRNOTAVAIL;
- goto err_iommu;
- }
- } else {
- eth_random_addr(vdpasim->config.mac);
- }
-
- vringh_set_iotlb(>vqs[0].vring, vdpasim->iommu);
- vringh_set_iotlb(>vqs[1].vring, vdpasim->iommu);
+ for (i = 0; i < VDPASIM_VQ_NUM; i++)
+ vringh_set_iotlb(>vqs[i].vring,
vdpasim->iommu);
And an attribute of #vqs here.
Yes.
vdpasim->vdpa.dma_dev = dev;
- ret = vdpa_register_device(>vdpa);
- if (ret)
- goto err_iommu;
return vdpasim;
@@ -404,6 +243,7 @@ static struct vdpasim *vdpasim_create(void)
err_alloc:
return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(vdpasim_create);
static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx,
u64 desc_area, u64 driver_area,
@@ -498,28 +338,34 @@ static u32 vdpasim_get_vq_align(struct vdpa_device *vdpa)
static u64 vdpasim_get_features(struct vdpa_device *vdpa)
{
- return vdpasim_features;
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+ return vdpasim->supported_features;
}
static int vdpasim_set_features(struct vdpa_device *vdpa, u64
features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- struct virtio_net_config *config = >config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return
Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules
On Mon, Nov 16, 2020 at 12:00:11PM +0800, Jason Wang wrote: On 2020/11/13 下午9:47, Stefano Garzarella wrote: From: Max Gurtovoy Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a preparation for adding a vdpa simulator module for block devices. Signed-off-by: Max Gurtovoy [sgarzare: various cleanups/fixes] Signed-off-by: Stefano Garzarella --- v1: - Removed unused headers - Removed empty module_init() module_exit() - Moved vdpasim_is_little_endian() in vdpa_sim.h - Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h - Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64 - Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected option can not depend on other [Jason] If possible, I would suggest to split this patch further: 1) convert to use void *config, and an attribute for setting config size during allocation 2) introduce supported_features 3) other attributes (#vqs) 4) rename config ops (more generic one) 5) introduce ops for set|get_config, set_get_features 6) real split Okay, I'll try to split Max's patch following your suggestion. It should be cleaner. Thanks, Stefano
Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules
On 2020/11/13 下午9:47, Stefano Garzarella wrote:
From: Max Gurtovoy
Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a
preparation for adding a vdpa simulator module for block devices.
Signed-off-by: Max Gurtovoy
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella
---
v1:
- Removed unused headers
- Removed empty module_init() module_exit()
- Moved vdpasim_is_little_endian() in vdpa_sim.h
- Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h
- Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
option can not depend on other [Jason]
If possible, I would suggest to split this patch further:
1) convert to use void *config, and an attribute for setting config size
during allocation
2) introduce supported_features
3) other attributes (#vqs)
4) rename config ops (more generic one)
5) introduce ops for set|get_config, set_get_features
6) real split
---
drivers/vdpa/vdpa_sim/vdpa_sim.h | 110 +++
drivers/vdpa/vdpa_sim/vdpa_sim.c | 285 ++-
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 153 ++
drivers/vdpa/Kconfig | 7 +-
drivers/vdpa/vdpa_sim/Makefile | 1 +
5 files changed, 329 insertions(+), 227 deletions(-)
create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim.h
create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim_net.c
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
new file mode 100644
index ..33613c49888c
--- /dev/null
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -0,0 +1,110 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2020, Red Hat Inc. All rights reserved.
+ */
+
+#ifndef _VDPA_SIM_H
+#define _VDPA_SIM_H
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_VERSION "0.1"
+#define DRV_AUTHOR "Jason Wang "
+#define DRV_LICENSE "GPL v2"
+
+#define VDPASIM_QUEUE_ALIGN PAGE_SIZE
+#define VDPASIM_QUEUE_MAX 256
+#define VDPASIM_VENDOR_ID 0
+#define VDPASIM_VQ_NUM 0x2
+
+#define VDPASIM_FEATURES ((1ULL << VIRTIO_F_ANY_LAYOUT) | \
+(1ULL << VIRTIO_F_VERSION_1) | \
+(1ULL << VIRTIO_F_ACCESS_PLATFORM))
+
+struct vdpasim;
+
+struct vdpasim_virtqueue {
+ struct vringh vring;
+ struct vringh_kiov iov;
+ unsigned short head;
+ bool ready;
+ u64 desc_addr;
+ u64 device_addr;
+ u64 driver_addr;
+ u32 num;
+ void *private;
+ irqreturn_t (*cb)(void *data);
+};
+
+struct vdpasim_init_attr {
+ u32 device_id;
+ u64 features;
+ work_func_t work_fn;
+ int batch_mapping;
+};
+
+/* State of each vdpasim device */
+struct vdpasim {
+ struct vdpa_device vdpa;
+ struct vdpasim_virtqueue vqs[VDPASIM_VQ_NUM];
+ struct work_struct work;
+ /* spinlock to synchronize virtqueue state */
+ spinlock_t lock;
+ /* virtio config according to device type */
+ void *config;
+ struct vhost_iotlb *iommu;
+ void *buffer;
+ u32 device_id;
+ u32 status;
+ u32 generation;
+ u64 features;
+ u64 supported_features;
+ /* spinlock to synchronize iommu table */
+ spinlock_t iommu_lock;
+};
+
+struct vdpasim *vdpasim_create(struct vdpasim_init_attr *attr);
+
+/* TODO: cross-endian support */
+static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
+{
+ return virtio_legacy_is_little_endian() ||
+ (vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+}
+
+static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
+{
+ return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
+{
+ return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline u32 vdpasim32_to_cpu(struct vdpasim *vdpasim, __virtio32 val)
+{
+ return __virtio32_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio32 cpu_to_vdpasim32(struct vdpasim *vdpasim, u32 val)
+{
+ return __cpu_to_virtio32(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline u64 vdpasim64_to_cpu(struct vdpasim *vdpasim, __virtio64 val)
+{
+ return __virtio64_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio64 cpu_to_vdpasim64(struct vdpasim *vdpasim, u64 val)
+{
+ return __cpu_to_virtio64(vdpasim_is_little_endian(vdpasim), val);
+}
+
+#endif
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 6a90fdb9cbfc..04f9dc9ce8c8 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -1,107 +1,16 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * VDPA networking device simulator.
+ * VDPA simulator core.
*
* Copyright (c)
