KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Jason Wang <jasowang@redhat.com>
To: Jason Gunthorpe <jgg@mellanox.com>
Cc: "mst@redhat.com" <mst@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"tiwei.bie@intel.com" <tiwei.bie@intel.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"cunming.liang@intel.com" <cunming.liang@intel.com>,
	"zhihong.wang@intel.com" <zhihong.wang@intel.com>,
	"rob.miller@broadcom.com" <rob.miller@broadcom.com>,
	"xiao.w.wang@intel.com" <xiao.w.wang@intel.com>,
	"haotian.wang@sifive.com" <haotian.wang@sifive.com>,
	"lingshan.zhu@intel.com" <lingshan.zhu@intel.com>,
	"eperezma@redhat.com" <eperezma@redhat.com>,
	"lulu@redhat.com" <lulu@redhat.com>,
	Parav Pandit <parav@mellanox.com>,
	"kevin.tian@intel.com" <kevin.tian@intel.com>,
	"stefanha@redhat.com" <stefanha@redhat.com>,
	"rdunlap@infradead.org" <rdunlap@infradead.org>,
	"hch@infradead.org" <hch@infradead.org>,
	"aadam@redhat.com" <aadam@redhat.com>,
	"jakub.kicinski@netronome.com" <jakub.kicinski@netronome.com>,
	Jiri Pirko <jiri@mellanox.com>,
	Shahaf Shuler <shahafs@mellanox.com>,
	"hanand@xilinx.com" <hanand@xilinx.com>,
	"mhabets@solarflare.com" <mhabets@solarflare.com>,
	kuba@kernel.org
Subject: Re: [PATCH 5/5] vdpasim: vDPA device simulator
Date: Fri, 17 Jan 2020 17:32:39 +0800
Message-ID: <aea2bff8-82c8-2c0f-19ee-e86db73e199f@redhat.com> (raw)
In-Reply-To: <20200116154658.GJ20978@mellanox.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
>


  reply index

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 12:42 [PATCH 0/5] vDPA support Jason Wang
2020-01-16 12:42 ` [PATCH 1/5] vhost: factor out IOTLB Jason Wang
2020-01-17  4:14   ` Randy Dunlap
2020-01-17  9:34     ` Jason Wang
2020-01-18  0:01   ` kbuild test robot
2020-01-18  0:40   ` kbuild test robot
2020-01-16 12:42 ` [PATCH 2/5] vringh: IOTLB support Jason Wang
2020-01-17 21:54   ` kbuild test robot
2020-01-17 22:33   ` kbuild test robot
2020-01-16 12:42 ` [PATCH 3/5] vDPA: introduce vDPA bus Jason Wang
2020-01-16 15:22   ` Jason Gunthorpe
2020-01-17  3:03     ` Jason Wang
2020-01-17 13:54       ` Jason Gunthorpe
2020-01-20  7:50         ` Jason Wang
2020-01-20 12:17         ` Michael S. Tsirkin
2020-01-20 17:50           ` Jason Gunthorpe
2020-01-20 21:56             ` Michael S. Tsirkin
2020-01-21 14:12               ` Jason Gunthorpe
2020-01-21 14:15                 ` Michael S. Tsirkin
2020-01-21 14:16                   ` Jason Gunthorpe
2020-01-21  8:40       ` Tian, Kevin
2020-01-21  9:41         ` Jason Wang
2020-01-17  4:16   ` Randy Dunlap
2020-01-17  9:34     ` Jason Wang
2020-01-17 12:13   ` Michael S. Tsirkin
2020-01-17 13:52     ` Jason Wang
     [not found]       ` <CAJPjb1+fG9L3=iKbV4Vn13VwaeDZZdcfBPvarogF_Nzhk+FnKg@mail.gmail.com>
2020-01-19  9:07         ` Shahaf Shuler
2020-01-19  9:59           ` Michael S. Tsirkin
2020-01-20  8:44             ` Jason Wang
2020-01-20 12:09               ` Michael S. Tsirkin
2020-01-21  3:32                 ` Jason Wang
2020-01-20  8:43           ` Jason Wang
2020-01-20 17:49             ` Jason Gunthorpe
2020-01-20 20:51               ` Shahaf Shuler
2020-01-20 21:25                 ` Michael S. Tsirkin
2020-01-20 21:47                   ` Shahaf Shuler
2020-01-20 21:59                     ` Michael S. Tsirkin
2020-01-21  6:01                       ` Shahaf Shuler
2020-01-21  7:57                         ` Jason Wang
2020-01-21 14:07                   ` Jason Gunthorpe
2020-01-21 14:16                     ` Michael S. Tsirkin
2020-01-20 21:48               ` Michael S. Tsirkin
2020-01-21  4:00               ` Jason Wang
2020-01-21  5:47                 ` Michael S. Tsirkin
2020-01-21  8:00                   ` Jason Wang
2020-01-21  8:15                     ` Michael S. Tsirkin
2020-01-21  8:35                       ` Jason Wang
2020-01-21 11:09                         ` Shahaf Shuler
2020-01-22  6:36                           ` Jason Wang
2020-01-21 14:05                       ` Jason Gunthorpe
2020-01-21 14:17                         ` Michael S. Tsirkin
2020-01-22  6:18                           ` Jason Wang
2020-01-20  8:19         ` Jason Wang
2020-01-16 12:42 ` [PATCH 4/5] virtio: introduce a vDPA based transport Jason Wang
2020-01-16 15:38   ` Jason Gunthorpe
2020-01-17  9:32     ` Jason Wang
2020-01-17 14:00       ` Jason Gunthorpe
2020-01-20  7:52         ` Jason Wang
2020-01-17  4:10   ` Randy Dunlap
2020-01-16 12:42 ` [PATCH 5/5] vdpasim: vDPA device simulator Jason Wang
2020-01-16 15:47   ` Jason Gunthorpe
2020-01-17  9:32     ` Jason Wang [this message]
2020-01-17 14:10       ` Jason Gunthorpe
2020-01-20  8:01         ` Jason Wang
2020-02-04  4:19         ` Jason Wang
2020-01-17  4:12   ` Randy Dunlap
2020-01-17  9:35     ` Jason Wang
2020-01-18 18:18   ` kbuild test robot
2020-01-28  3:32   ` Dan Carpenter
2020-02-04  4:07     ` Jason Wang
2020-02-04  8:21   ` Zhu Lingshan
2020-02-04  8:28     ` Jason Wang
2020-02-04 12:52       ` Jason Gunthorpe
2020-02-05  3:14         ` Jason Wang
2020-01-21  8:44 ` [PATCH 0/5] vDPA support Tian, Kevin
2020-01-21  9:39   ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aea2bff8-82c8-2c0f-19ee-e86db73e199f@redhat.com \
    --to=jasowang@redhat.com \
    --cc=aadam@redhat.com \
    --cc=cunming.liang@intel.com \
    --cc=eperezma@redhat.com \
    --cc=hanand@xilinx.com \
    --cc=haotian.wang@sifive.com \
    --cc=hch@infradead.org \
    --cc=jakub.kicinski@netronome.com \
    --cc=jgg@mellanox.com \
    --cc=jiri@mellanox.com \
    --cc=kevin.tian@intel.com \
    --cc=kuba@kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=lingshan.zhu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lulu@redhat.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=mhabets@solarflare.com \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=parav@mellanox.com \
    --cc=rdunlap@infradead.org \
    --cc=rob.miller@broadcom.com \
    --cc=shahafs@mellanox.com \
    --cc=stefanha@redhat.com \
    --cc=tiwei.bie@intel.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=xiao.w.wang@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git