On Thu, Jan 16, 2020 at 08:42:31PM +0800, Jason Wang wrote:

This patch implements a software vDPA networking device. The datapath
is implemented through vringh and workqueue. The device has an on-chip
IOMMU which translates IOVA to PA. For kernel virtio drivers, vDPA
simulator driver provides dma_ops. For vhost driers, set_map() methods
of vdpa_config_ops is implemented to accept mappings from vhost.

A sysfs based management interface is implemented, devices are
created and removed through:

/sys/devices/virtual/vdpa_simulator/netdev/{create|remove}

This is very gross, creating a class just to get a create/remove and
then not using the class for anything else? Yuk.

Netlink based lifecycle management could be implemented for vDPA
simulator as well.

This is just begging for a netlink based approach.

Certainly netlink driven removal should be an agreeable standard for
all devices, I think.

+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_CLASS_NAME "vdpa_simulator"
+#define VDPASIM_NAME "netdev"
+
+u64 vdpasim_features = (1ULL << VIRTIO_F_ANY_LAYOUT) |
+		       (1ULL << VIRTIO_F_VERSION_1)  |
+		       (1ULL << VIRTIO_F_IOMMU_PLATFORM);

Is not using static here intentional?

+static void vdpasim_release_dev(struct device *_d)
+{
+	struct vdpa_device *vdpa = dev_to_vdpa(_d);
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+	sysfs_remove_link(vdpasim_dev->devices_kobj, vdpasim->name);
+
+	mutex_lock(&vsim_list_lock);
+	list_del(&vdpasim->next);
+	mutex_unlock(&vsim_list_lock);
+
+	kfree(vdpasim->buffer);
+	kfree(vdpasim);
+}

It is again a bit weird to see a realease function in a driver. This
stuff is usually in the remove remove function.

+static int vdpasim_create(const guid_t *uuid)
+{
+	struct vdpasim *vdpasim, *tmp;
+	struct virtio_net_config *config;
+	struct vdpa_device *vdpa;
+	struct device *dev;
+	int ret = -ENOMEM;
+
+	mutex_lock(&vsim_list_lock);
+	list_for_each_entry(tmp, &vsim_devices_list, next) {
+		if (guid_equal(&tmp->uuid, uuid)) {
+			mutex_unlock(&vsim_list_lock);
+			return -EEXIST;
+		}
+	}
+
+	vdpasim = kzalloc(sizeof(*vdpasim), GFP_KERNEL);
+	if (!vdpasim)
+		goto err_vdpa_alloc;
+
+	vdpasim->buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!vdpasim->buffer)
+		goto err_buffer_alloc;
+
+	vdpasim->iommu = vhost_iotlb_alloc(2048, 0);
+	if (!vdpasim->iommu)
+		goto err_iotlb;
+
+	config = &vdpasim->config;
+	config->mtu = 1500;
+	config->status = VIRTIO_NET_S_LINK_UP;
+	eth_random_addr(config->mac);
+
+	INIT_WORK(&vdpasim->work, vdpasim_work);
+	spin_lock_init(&vdpasim->lock);
+
+	guid_copy(&vdpasim->uuid, uuid);
+
+	list_add(&vdpasim->next, &vsim_devices_list);
+	vdpa = &vdpasim->vdpa;
+
+	mutex_unlock(&vsim_list_lock);
+
+	vdpa = &vdpasim->vdpa;
+	vdpa->config = &vdpasim_net_config_ops;
+	vdpa_set_parent(vdpa, &vdpasim_dev->dev);
+	vdpa->dev.release = vdpasim_release_dev;
+
+	vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
+	vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
+
+	dev = &vdpa->dev;
+	dev->coherent_dma_mask = DMA_BIT_MASK(64);
+	set_dma_ops(dev, &vdpasim_dma_ops);
+
+	ret = register_vdpa_device(vdpa);
+	if (ret)
+		goto err_register;
+
+	sprintf(vdpasim->name, "%pU", uuid);
+
+	ret = sysfs_create_link(vdpasim_dev->devices_kobj, &vdpa->dev.kobj,
+				vdpasim->name);
+	if (ret)
+		goto err_link;

The goto err_link does the wrong unwind, once register is completed
the error unwind is unregister & put_device, not kfree. This is why I
recommend to always initalize the device early, and always using
put_device during error unwinds.

This whole guid thing seems unncessary when the device is immediately
assigned a vdpa index from the ida. If you were not using syfs you'd
just return that index from the creation netlink.

Jason