Re: [dpdk-dev] [PATCH v3 1/2] virtio/vdev: add embeded device emulation

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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