Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules

Stefano Garzarella
2020-11-18T13:14:22Z

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