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
