On 2020/1/16 下午11:47, Jason Gunthorpe wrote:
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.
It includes more information, e.g the devices and the link from vdpa_sim
device and vdpa device.
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.
Well, I think Parav had some proposals during the discussion of mdev
approach. But I'm not sure if he had any RFC codes for me to integrate
it into vdpasim.
Or do you want me to propose the netlink API? If yes, would you prefer
to a new virtio dedicated one or be a subset of devlink?
But it might be better to reach an agreement for all the vendors here.
Rob, Steve, Tiwei, Lingshan, Harpreet, Martin, Jakub, please share your
thoughts about the management API here.
+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?
No, let me fix.
+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.
Will fix.
+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.
Will fix.
This whole guid thing seems unncessary when the device is immediately
assigned a vdpa index from the ida.
The problem here is that user need to know which vdpa_sim is the one
that is just created.
If you were not using syfs you'd
just return that index from the creation netlink.
Yes it is.
Thanks
Jason
