Messages in this thread | | | Date | Wed, 18 Nov 2020 14:14:08 +0100 | From | Stefano Garzarella <> | Subject | 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 <mgurtovoy@nvidia.com> >> >>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 <mgurtovoy@nvidia.com> >>[sgarzare: various cleanups/fixes] >>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>--- >>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 <jasowang@redhat.com> >>+ * >>+ */ >>+ >>+#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
| |