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

[Jason Wang]

On 2020/11/13 下午9:47, Stefano Garzarella wrote:
> From: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> [sgarzare: various cleanups/fixes]
> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> ---
> 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 000000000000..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 <linux/vringh.h>
> +#include <linux/vdpa.h>
> +#include <linux/vhost_iotlb.h>
> +#include <uapi/linux/virtio_config.h>
> +#include <uapi/linux/virtio_net.h>
> +#include <uapi/linux/virtio_blk.h>
> +
> +#define DRV_VERSION  "0.1"
> +#define DRV_AUTHOR   "Jason Wang <jasowang@xxxxxxxxxx>"
> +#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) 2020, Red Hat Inc. All rights reserved.
>    *     Author: Jason Wang <jasowang@xxxxxxxxxx>
>    *
>    */
>   
> -#include <linux/init.h>
>   #include <linux/module.h>
> -#include <linux/device.h>
> -#include <linux/kernel.h>
> -#include <linux/fs.h>
> -#include <linux/poll.h>
> -#include <linux/slab.h>
> -#include <linux/sched.h>
> -#include <linux/wait.h>
> -#include <linux/uuid.h>
> -#include <linux/iommu.h>
>   #include <linux/dma-map-ops.h>
> -#include <linux/sysfs.h>
> -#include <linux/file.h>
> -#include <linux/etherdevice.h>
> -#include <linux/vringh.h>
> -#include <linux/vdpa.h>
> -#include <linux/virtio_byteorder.h>
> -#include <linux/vhost_iotlb.h>
> -#include <uapi/linux/virtio_config.h>
> -#include <uapi/linux/virtio_net.h>
> -
> -#define DRV_VERSION  "0.1"
> -#define DRV_AUTHOR   "Jason Wang <jasowang@xxxxxxxxxx>"
> -#define DRV_DESC     "vDPA Device Simulator"
> -#define DRV_LICENSE  "GPL v2"
> -
> -static int batch_mapping = 1;
> -module_param(batch_mapping, int, 0444);
> -MODULE_PARM_DESC(batch_mapping, "Batched mapping 1 -Enable; 0 - Disable");
> -
> -static char *macaddr;
> -module_param(macaddr, charp, 0);
> -MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
> -
> -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);
> -};
> -
> -#define VDPASIM_QUEUE_ALIGN PAGE_SIZE
> -#define VDPASIM_QUEUE_MAX 256
> -#define VDPASIM_DEVICE_ID 0x1
> -#define VDPASIM_VENDOR_ID 0
> -#define VDPASIM_VQ_NUM 0x2
> -#define VDPASIM_NAME "vdpasim-netdev"
> -
> -static u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) |
> -			      (1ULL << VIRTIO_F_VERSION_1)  |
> -			      (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> -			      (1ULL << VIRTIO_NET_F_MAC);
> -
> -/* 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;
> -	struct virtio_net_config config;
> -	struct vhost_iotlb *iommu;
> -	void *buffer;
> -	u32 status;
> -	u32 generation;
> -	u64 features;
> -	/* spinlock to synchronize iommu table */
> -	spinlock_t iommu_lock;
> -};
> -
> -/* 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 struct vdpasim *vdpasim_dev;
> +#include "vdpa_sim.h"
>   
>   static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
>   {
> @@ -119,7 +28,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>   {
>   	struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
>   
> -	vringh_init_iotlb(&vq->vring, vdpasim_features,
> +	vringh_init_iotlb(&vq->vring, vdpasim->supported_features,
>   			  VDPASIM_QUEUE_MAX, false,
>   			  (struct vring_desc *)(uintptr_t)vq->desc_addr,
>   			  (struct vring_avail *)
> @@ -128,7 +37,8 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>   			  (uintptr_t)vq->device_addr);
>   }
>   
> -static void vdpasim_vq_reset(struct vdpasim_virtqueue *vq)
> +static void vdpasim_vq_reset(struct vdpasim *vdpasim,
> +			     struct vdpasim_virtqueue *vq)
>   {
>   	vq->ready = false;
>   	vq->desc_addr = 0;
> @@ -136,8 +46,8 @@ static void vdpasim_vq_reset(struct vdpasim_virtqueue *vq)
>   	vq->device_addr = 0;
>   	vq->cb = NULL;
>   	vq->private = NULL;
> -	vringh_init_iotlb(&vq->vring, vdpasim_features, VDPASIM_QUEUE_MAX,
> -			  false, NULL, NULL, NULL);
> +	vringh_init_iotlb(&vq->vring, vdpasim->supported_features,
> +			  VDPASIM_QUEUE_MAX, false, NULL, NULL, NULL);
>   }
>   
>   static void vdpasim_reset(struct vdpasim *vdpasim)
> @@ -145,7 +55,7 @@ static void vdpasim_reset(struct vdpasim *vdpasim)
>   	int i;
>   
>   	for (i = 0; i < VDPASIM_VQ_NUM; i++)
> -		vdpasim_vq_reset(&vdpasim->vqs[i]);
> +		vdpasim_vq_reset(vdpasim, &vdpasim->vqs[i]);
>   
>   	spin_lock(&vdpasim->iommu_lock);
>   	vhost_iotlb_reset(vdpasim->iommu);
> @@ -156,80 +66,6 @@ static void vdpasim_reset(struct vdpasim *vdpasim)
>   	++vdpasim->generation;
>   }
>   
> -static void vdpasim_work(struct work_struct *work)
> -{
> -	struct vdpasim *vdpasim = container_of(work, struct
> -						 vdpasim, work);
> -	struct vdpasim_virtqueue *txq = &vdpasim->vqs[1];
> -	struct vdpasim_virtqueue *rxq = &vdpasim->vqs[0];
> -	ssize_t read, write;
> -	size_t total_write;
> -	int pkts = 0;
> -	int err;
> -
> -	spin_lock(&vdpasim->lock);
> -
> -	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -		goto out;
> -
> -	if (!txq->ready || !rxq->ready)
> -		goto out;
> -
> -	while (true) {
> -		total_write = 0;
> -		err = vringh_getdesc_iotlb(&txq->vring, &txq->iov, NULL,
> -					   &txq->head, GFP_ATOMIC);
> -		if (err <= 0)
> -			break;
> -
> -		err = vringh_getdesc_iotlb(&rxq->vring, NULL, &rxq->iov,
> -					   &rxq->head, GFP_ATOMIC);
> -		if (err <= 0) {
> -			vringh_complete_iotlb(&txq->vring, txq->head, 0);
> -			break;
> -		}
> -
> -		while (true) {
> -			read = vringh_iov_pull_iotlb(&txq->vring, &txq->iov,
> -						     vdpasim->buffer,
> -						     PAGE_SIZE);
> -			if (read <= 0)
> -				break;
> -
> -			write = vringh_iov_push_iotlb(&rxq->vring, &rxq->iov,
> -						      vdpasim->buffer, read);
> -			if (write <= 0)
> -				break;
> -
> -			total_write += write;
> -		}
> -
> -		/* Make sure data is wrote before advancing index */
> -		smp_wmb();
> -
> -		vringh_complete_iotlb(&txq->vring, txq->head, 0);
> -		vringh_complete_iotlb(&rxq->vring, rxq->head, total_write);
> -
> -		/* Make sure used is visible before rasing the interrupt. */
> -		smp_wmb();
> -
> -		local_bh_disable();
> -		if (txq->cb)
> -			txq->cb(txq->private);
> -		if (rxq->cb)
> -			rxq->cb(rxq->private);
> -		local_bh_enable();
> -
> -		if (++pkts > 4) {
> -			schedule_work(&vdpasim->work);
> -			goto out;
> -		}
> -	}
> -
> -out:
> -	spin_unlock(&vdpasim->lock);
> -}
> -
>   static int dir_to_perm(enum dma_data_direction dir)
>   {
>   	int perm = -EFAULT;
> @@ -342,26 +178,42 @@ static const struct dma_map_ops vdpasim_dma_ops = {
>   	.free = vdpasim_free_coherent,
>   };
>   
> -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?


> +	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.


>   
>   	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?


>   
> -	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?


>   }
>   
>   static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
> @@ -659,9 +511,10 @@ static void vdpasim_free(struct vdpa_device *vdpa)
>   	kfree(vdpasim->buffer);
>   	if (vdpasim->iommu)
>   		vhost_iotlb_free(vdpasim->iommu);
> +	kfree(vdpasim->config);
>   }
>   
> -static const struct vdpa_config_ops vdpasim_net_config_ops = {
> +static const struct vdpa_config_ops vdpasim_config_ops = {
>   	.set_vq_address         = vdpasim_set_vq_address,
>   	.set_vq_num             = vdpasim_set_vq_num,
>   	.kick_vq                = vdpasim_kick_vq,
> @@ -688,7 +541,7 @@ static const struct vdpa_config_ops vdpasim_net_config_ops = {
>   	.free                   = vdpasim_free,
>   };
>   
> -static const struct vdpa_config_ops vdpasim_net_batch_config_ops = {
> +static const struct vdpa_config_ops vdpasim_batch_config_ops = {
>   	.set_vq_address         = vdpasim_set_vq_address,
>   	.set_vq_num             = vdpasim_set_vq_num,
>   	.kick_vq                = vdpasim_kick_vq,
> @@ -714,27 +567,7 @@ static const struct vdpa_config_ops vdpasim_net_batch_config_ops = {
>   	.free                   = vdpasim_free,
>   };
>   
> -static int __init vdpasim_dev_init(void)
> -{
> -	vdpasim_dev = vdpasim_create();
> -
> -	if (!IS_ERR(vdpasim_dev))
> -		return 0;
> -
> -	return PTR_ERR(vdpasim_dev);
> -}
> -
> -static void __exit vdpasim_dev_exit(void)
> -{
> -	struct vdpa_device *vdpa = &vdpasim_dev->vdpa;
> -
> -	vdpa_unregister_device(vdpa);
> -}
> -
> -module_init(vdpasim_dev_init)
> -module_exit(vdpasim_dev_exit)
> -
>   MODULE_VERSION(DRV_VERSION);
>   MODULE_LICENSE(DRV_LICENSE);
>   MODULE_AUTHOR(DRV_AUTHOR);
> -MODULE_DESCRIPTION(DRV_DESC);
> +MODULE_DESCRIPTION("vDPA Simulator core");
> 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@xxxxxxxxxx>
> + *
> + */
> +
> +#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.


> +
> +static char *macaddr;
> +module_param(macaddr, charp, 0);
> +MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
> +
> +static struct vdpasim *vdpasim_net_dev;
> +
> +static void vdpasim_net_work(struct work_struct *work)
> +{
> +	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> +	struct vdpasim_virtqueue *txq = &vdpasim->vqs[1];
> +	struct vdpasim_virtqueue *rxq = &vdpasim->vqs[0];
> +	ssize_t read, write;
> +	size_t total_write;
> +	int pkts = 0;
> +	int err;
> +
> +	spin_lock(&vdpasim->lock);
> +
> +	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +		goto out;
> +
> +	if (!txq->ready || !rxq->ready)
> +		goto out;
> +
> +	while (true) {
> +		total_write = 0;
> +		err = vringh_getdesc_iotlb(&txq->vring, &txq->iov, NULL,
> +					   &txq->head, GFP_ATOMIC);
> +		if (err <= 0)
> +			break;
> +
> +		err = vringh_getdesc_iotlb(&rxq->vring, NULL, &rxq->iov,
> +					   &rxq->head, GFP_ATOMIC);
> +		if (err <= 0) {
> +			vringh_complete_iotlb(&txq->vring, txq->head, 0);
> +			break;
> +		}
> +
> +		while (true) {
> +			read = vringh_iov_pull_iotlb(&txq->vring, &txq->iov,
> +						     vdpasim->buffer,
> +						     PAGE_SIZE);
> +			if (read <= 0)
> +				break;
> +
> +			write = vringh_iov_push_iotlb(&rxq->vring, &rxq->iov,
> +						      vdpasim->buffer, read);
> +			if (write <= 0)
> +				break;
> +
> +			total_write += write;
> +		}
> +
> +		/* Make sure data is wrote before advancing index */
> +		smp_wmb();
> +
> +		vringh_complete_iotlb(&txq->vring, txq->head, 0);
> +		vringh_complete_iotlb(&rxq->vring, rxq->head, total_write);
> +
> +		/* Make sure used is visible before rasing the interrupt. */
> +		smp_wmb();
> +
> +		local_bh_disable();
> +		if (txq->cb)
> +			txq->cb(txq->private);
> +		if (rxq->cb)
> +			rxq->cb(rxq->private);
> +		local_bh_enable();
> +
> +		if (++pkts > 4) {
> +			schedule_work(&vdpasim->work);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	spin_unlock(&vdpasim->lock);
> +}
> +
> +static int __init vdpasim_net_init(void)
> +{
> +	struct vdpasim_init_attr attr = {};
> +	struct virtio_net_config *config;
> +	int ret;
> +
> +	attr.device_id = VIRTIO_ID_NET;
> +	attr.features = VDPASIM_FEATURES | VDPASIM_NET_FEATURES;
> +	attr.work_fn = vdpasim_net_work;
> +	attr.batch_mapping = batch_mapping;
> +	vdpasim_net_dev = vdpasim_create(&attr);
> +	if (IS_ERR(vdpasim_net_dev)) {
> +		ret = PTR_ERR(vdpasim_net_dev);
> +		goto out;
> +	}
> +
> +	config = (struct virtio_net_config *)vdpasim_net_dev->config;
> +
> +	if (macaddr) {
> +		mac_pton(macaddr, config->mac);
> +		if (!is_valid_ether_addr(config->mac)) {
> +			ret = -EADDRNOTAVAIL;
> +			goto put_dev;
> +		}
> +	} else {
> +		eth_random_addr(config->mac);
> +	}
> +
> +	ret = vdpa_register_device(&vdpasim_net_dev->vdpa);
> +	if (ret)
> +		goto put_dev;
> +
> +	return 0;
> +
> +put_dev:
> +	put_device(&vdpasim_net_dev->vdpa.dev);
> +out:
> +	return ret;
> +}
> +
> +static void __exit vdpasim_net_exit(void)
> +{
> +	struct vdpa_device *vdpa = &vdpasim_net_dev->vdpa;
> +
> +	vdpa_unregister_device(vdpa);
> +}
> +
> +module_init(vdpasim_net_init);
> +module_exit(vdpasim_net_exit);
> +
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_LICENSE(DRV_LICENSE);
> +MODULE_AUTHOR(DRV_AUTHOR);
> +MODULE_DESCRIPTION("vDPA Device Simulator for networking device");
> 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.

Thanks


>   	help
>   	  vDPA networking device simulator which loop TX traffic back
>   	  to RX. This device is used for testing, prototyping and
> diff --git a/drivers/vdpa/vdpa_sim/Makefile b/drivers/vdpa/vdpa_sim/Makefile
> index b40278f65e04..79d4536d347e 100644
> --- a/drivers/vdpa/vdpa_sim/Makefile
> +++ b/drivers/vdpa/vdpa_sim/Makefile
> @@ -1,2 +1,3 @@
>   # SPDX-License-Identifier: GPL-2.0
>   obj-$(CONFIG_VDPA_SIM) += vdpa_sim.o
> +obj-$(CONFIG_VDPA_SIM_NET) += vdpa_sim_net.o