Re: [PATCH 5/5] vdpasim: vDPA device simulator

[Jason Wang <jasowang@redhat.com>]

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
>