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 <[email protected]>
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 <[email protected]>
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella <[email protected]>
---
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 = &vdpasim_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 = &vdpasim_batch_config_ops;
else
- ops = &vdpasim_net_config_ops;
+ ops = &vdpasim_config_ops;
vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops,
VDPASIM_VQ_NUM);
if (!vdpasim)
goto err_alloc;
- INIT_WORK(&vdpasim->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(&vdpasim->work, attr->work_fn);
spin_lock_init(&vdpasim->lock);
spin_lock_init(&vdpasim->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(&vdpasim->vqs[0].vring, vdpasim->iommu);
- vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
+ for (i = 0; i < VDPASIM_VQ_NUM; i++)
+ vringh_set_iotlb(&vdpasim->vqs[i].vring,
vdpasim->iommu);
And an attribute of #vqs here.
Yes.
vdpasim->vdpa.dma_dev = dev;
- ret = vdpa_register_device(&vdpasim->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 = &vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return -EINVAL;
- vdpasim->features = features & vdpasim_features;
+ vdpasim->features = features & vdpasim->supported_features;
/* We generally only know whether guest is using the legacy interface
* here, so generally that's the earliest we can set config fields.
* Note: We actually require VIRTIO_F_ACCESS_PLATFORM above which
* implies VIRTIO_F_VERSION_1, but let's not try to be clever here.
*/
+ if (vdpasim->device_id == VIRTIO_ID_NET) {
+ struct virtio_net_config *config =
+ (struct virtio_net_config *)vdpasim->config;
+
+ config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
+ config->status = cpu_to_vdpasim16(vdpasim,
VIRTIO_NET_S_LINK_UP);
+ }
Can we introduce callbacks of set_features/get_features here to avoid
dealing of device type specific codes in generic simulator code?
Yes, I'll do.
- config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
- config->status = cpu_to_vdpasim16(vdpasim,
VIRTIO_NET_S_LINK_UP);
return 0;
}
@@ -536,7 +382,9 @@ static u16 vdpasim_get_vq_num_max(struct
vdpa_device *vdpa)
static u32 vdpasim_get_device_id(struct vdpa_device *vdpa)
{
- return VDPASIM_DEVICE_ID;
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+ return vdpasim->device_id;
}
static u32 vdpasim_get_vendor_id(struct vdpa_device *vdpa)
@@ -572,8 +420,12 @@ static void vdpasim_get_config(struct vdpa_device
*vdpa, unsigned int offset,
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- if (offset + len < sizeof(struct virtio_net_config))
- memcpy(buf, (u8 *)&vdpasim->config + offset, len);
+ if (vdpasim->device_id == VIRTIO_ID_BLOCK &&
+ (offset + len < sizeof(struct virtio_blk_config)))
+ memcpy(buf, vdpasim->config + offset, len);
+ else if (vdpasim->device_id == VIRTIO_ID_NET &&
+ (offset + len < sizeof(struct virtio_net_config)))
+ memcpy(buf, vdpasim->config + offset, len);
Similarly, can we introduce set/get_config ops?
Ditto.
[...]
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
new file mode 100644
index 000000000000..c68d5488ab54
--- /dev/null
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VDPA simulator for networking device.
+ *
+ * Copyright (c) 2020, Red Hat Inc. All rights reserved.
+ * Author: Jason Wang <[email protected]>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/etherdevice.h>
+
+#include "vdpa_sim.h"
+
+#define VDPASIM_NET_FEATURES (1ULL << VIRTIO_NET_F_MAC)
+
+static int batch_mapping = 1;
+module_param(batch_mapping, int, 0444);
+MODULE_PARM_DESC(batch_mapping, "Batched mapping 1 - Enable; 0 - Disable");
I think batch_mapping should belong to vpda_sim core module.
Yes, I agree, I'll leave it in the core.
[...]
diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
index d7d32b656102..fdb1a9267347 100644
--- a/drivers/vdpa/Kconfig
+++ b/drivers/vdpa/Kconfig
@@ -9,11 +9,16 @@ menuconfig VDPA
if VDPA
config VDPA_SIM
- tristate "vDPA device simulator"
+ tristate "vDPA simulator core"
depends on RUNTIME_TESTING_MENU && HAS_DMA
select DMA_OPS
select VHOST_RING
default n
+
+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?
Thanks,
Stefano
