From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 5615A2904 for ; Fri, 22 Apr 2016 19:25:49 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 22 Apr 2016 10:25:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,518,1455004800"; d="scan'208";a="964438049" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga002.fm.intel.com with ESMTP; 22 Apr 2016 10:25:46 -0700 Date: Fri, 22 Apr 2016 10:27:25 -0700 From: Yuanhan Liu To: "Tan, Jianfeng" Cc: dev@dpdk.org, Huawei Xie , rich.lane@bigswitch.com, mst@redhat.com, nakajima.yoshihiro@lab.ntt.co.jp, p.fedin@samsung.com, michael.qiu@intel.com, ann.zhuangyanying@huawei.com, mukawa@igel.co.jp, nhorman@tuxdrver.com, Thomas Monjalon Message-ID: <20160422172725.GD7603@yliu-dev.sh.intel.com> References: <1446748276-132087-1-git-send-email-jianfeng.tan@intel.com> <1461207396-42742-1-git-send-email-jianfeng.tan@intel.com> <1461207396-42742-2-git-send-email-jianfeng.tan@intel.com> <20160421220121.GA7603@yliu-dev.sh.intel.com> <5719F905.1060704@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5719F905.1060704@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio/vdev: add embeded device emulation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Apr 2016 17:25:50 -0000 On Fri, Apr 22, 2016 at 06:12:21PM +0800, Tan, Jianfeng wrote: > >>How: we implement another instance of struct virtio_pci_ops to intercept > >>the communications between VM and QEMU. Instead of rd/wr ioport or PCI > >>configuration space, here we directly talk with backend through the vhost > >>file. > >Nope, that's wrong, and here becomes a bit subtle. I will try to make > >some explanation here. > > > >Let's talk about the normal case (with QEMU) first. Where, virtio PMD > >is a driver, and virito device is emulated inside QEMU, and exposed by > >PCI. So, virtio PMD talks to the device with ioport rd/wr (or MMIO for > >virtio 1.0). > > > >Till now, you are right. > > > >However, vhost-user socket is for establishing a connection, providing > >HOST with enough information, so that host can directly manipulate the > >vring, to dequeue/enqueue buffers. > > > >So, what you were saying about "directly talk with backend (the virtual > >virtio device you created) through vhost file" is not right. Instead, > >in your case, the (virtual) virtio device and the PMD driver is in > >the same process space, therefore, you could actually access or the > >device info simply by normal read/write. > > Here by _backend_, I mean vhost. I know you take backend as the device > emulation. Yes, because I stated it explicitly (by saying "the virtual device you created"). And here is why. Let's check your commit log first: Instead of rd/wr ioport or PCI configuration space, here we directly talk with backend through the vhost file. Firstly, assume the backend is "vhost-user backend" as you stated, we are always using vhost file to talk to vhost-user backend, aren't we? So, I assume you were stating backend as the "virtual device". However, here you were comparing two __different__ things. The first half you were talking about guest and driver communication, the later half you were saying the communication between vhost-user frontend and backend. > I ask Huawei, he thinks backend = device emulation + vhost. So I will not say he is wrong. But they are actually two different thing. Device emulation is for, as it's name explains, emulating the virtio device, yet it handles the communication from the driver. Vhost-user is for implementing the frontend, initiating the negoitation to the vhost-user backend. See the difference? It might be subtle somehow, but the difference is obvious. You should seperate them. Now let's look at some of your code: +static uint64_t +vdev_get_features(struct virtio_hw *hw) +{ + uint64_t host_features; + + vhost_call(hw, VHOST_MSG_GET_FEATURES, &host_features); + if (hw->mac_specified) + host_features |= (1ull << VIRTIO_NET_F_MAC); + /* disable it until we support CQ */ + host_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ); + host_features &= ~(1ull << VIRTIO_NET_F_CTRL_RX); + return host_features; +} The get_features method is actually for getting the features supported by the __device__. Therefore, you should maintain a standalone feature list in your virtual device implementation (like what QEMU does). However, here you directly fetch the features from the vhost-user backend. One more example: +static void +vdev_set_features(struct virtio_hw *hw __rte_unused, + uint64_t features __rte_unused) +{ + /* We hold to send this message, VHOST_MSG_SET_FEATURES, + * until all VirtQueues have been kicked. + */ +} I know why you were doing that, you also made a comment. The fact of the matter is that you should not do that (send vhost-user request). "set_features" method is for setting the negoiated features from driver and device, and then store it in virtual device. You should not touch vhost-user backend here. So, sorry, you are actually doing what you stated: doing device configuration through vhost-user interface. But that's a wrong way to make things work. The right way to go is to maintain a struct inside the virtual device, to keep all related states, such as features, status. And do vhost-user negotiation just only when VIRTIO_CONFIG_S_DRIVER_OK is seen. In this way, you could keep the logic separately. That's one of the reason why I ask you to separate them in different files. > maybe I should use the phrase vhost for accuracy. > > > > >As you can see, It's a bit messy to mix all of them (virtio PMD driver, > >virtio device emulation, and vhost-uesr frontend) in one single directory > >(or even in one single file as you did). Therefore, I'd suggest you to > >make a new dir, say "virtio-user" (a good name from Thomas), and put all > >files related to virtio device emulation and vhost-user frontend there. > > > >Further more, I'd suggest to divide the code into following files: > > > >- virtio-user/virtio.c > > > > All virtio device emulation goes here. > > > >- virtio-user/vhost-user.c > > > > The vhost-user frontend implementation > > > >- virtio-user/vhost-kernel.c > > > > vhost kernel hanldings, including setting the tap device. > > > >- And, __maybe__ another standalone file for handling the talk > > between the driver and the device. (See more for the comments > > about virtio_pci_ops below). > > > > > >That would make it much clearer, IMO. > > Got your point here, but to be honest, I'm a little concerned to split a > 1k-lined file into 5+ files. What's wrong doing that? Does it hurt anything? Nope, instead, I see many benefits while doing that: clean code/logic/design, easier for review, easier for maintain, and so on... I just see it hurts a lot when not doing that. Don't even to you say you are going to add more stuff based on that, say control queue. > Previously I'd like to make them converged > together. But your suggestion is great for clean code. If any other could > give some comments? Hmmmm, since you were admitting it's "great for clean code", I'm just wondering why this is still not enough to convince you??? It's more than making things work; it's more important to make things work in the right/clean way. > >>+ bzero(&msgh, sizeof(msgh)); > >>+ bzero(control, sizeof(control)); > >bzero is marked as deprecated (see the man page), use memset instead. > > I should apologize about this, you actually told me once before, but I > failed to fix it in this version. Never mind. It's been for a while, we (as human) just forgot things. > >>diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > >>index b69785e..68097e6 100644 > >>--- a/drivers/net/virtio/virtio_pci.h > >>+++ b/drivers/net/virtio/virtio_pci.h > >>@@ -260,6 +260,20 @@ struct virtio_hw { > >> struct virtio_pci_common_cfg *common_cfg; > >> struct virtio_net_config *dev_cfg; > >> const struct virtio_pci_ops *vtpci_ops; > >>+#ifdef RTE_VIRTIO_VDEV > >>+#define VHOST_KERNEL 0 > >>+#define VHOST_USER 1 > >>+ int type; /* type of backend */ > >>+ uint32_t queue_num; > >>+ char *path; > >>+ int mac_specified; > >>+ int vhostfd; > >>+ int backfd; /* tap device used in vhost-net */ > >>+ int callfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1]; > >>+ int kickfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1]; > >>+ uint8_t status; > >>+ struct rte_eth_dev_data *data; > >>+#endif > >And put all of them to the virtio "device" context, in the virtio-user/ > >then. > > OK, I'll define a new struct to store these fields. I consider there are two > methods to refer these data. One is to store a pointer to the instance of > the new struct in the struct virtio_hw; the other is to main them totally > closed inside virtio-user, such as using a linked chain. Which do you > prefer? Let's try the first one first; it's simpler, and let's see how well it might work. If not, we could try others. --yliu