[dpdk-dev] [PATCH v4 6/8] virtio-user: add new virtual pci driver for virtio

[Yuanhan Liu]

On Thu, May 12, 2016 at 03:08:05PM +0800, Tan, Jianfeng wrote:
> >>+static int
> >>+vdev_setup_queue(struct virtio_hw *hw __rte_unused, struct virtqueue *vq)
> >>+{
> >>+	/* Changed to use virtual addr */
> >>+	vq->vq_ring_mem = (phys_addr_t)vq->mz->addr;
> >>+	if (vq->virtio_net_hdr_mz) {
> >>+		vq->virtio_net_hdr_mem =
> >>+			(phys_addr_t)vq->virtio_net_hdr_mz->addr;
> >>+		/* Do it one more time after we reset virtio_net_hdr_mem */
> >>+		vring_hdr_desc_init(vq);
> >>+	}
> >>+	vq->offset = offsetof(struct rte_mbuf, buf_addr);
> >>+	return 0;
> >Here as last email said, you should not mix vq stuff. What's more,
> >why do you invoke vring_hdr_desc_init() here?
> 
> vring_hdr_desc_init() is to init header desc according to
> vq->virtio_net_hdr_mem, and here we change to use virtual address, so we
> need to invoke this after vq->virtio_net_hdr_mem is decided.
> 
> But for this case, you remind me that we can achieve that by: inside
> virtio_dev_queue_setup(), move vring_hdr_desc_init() after setup_queue().
> 
> >If it needs a special
> >handling, do it in driver.
> 
> As discussed in previous mail with David, we should hide special handling
> inside pci ops,

Generally speaking, yes.

> such as real virtio device needs to check address (patch 1).

And that's a good one: I've already acked. But here, I doubt it introduces 
any benefits to do that. Firstly, it's a Tx queue specific setting, moving
it to common code path means you have to add a check like the way you did
in this patch. BTW, it's an implicit check, which hurts readability a bit.

Secondly, you have to do this kind of check/settings in 3 different places: 

- legacy queue_setup() method
- modern queue_setup() method
- your vdev queue_setup() method

And another remind is that Huawei planned to split Rx/Tx queue settings,
here you mixed them again, and I don't think Huawei would like it. Don't
even to say that after the split, the Tx specific stuff will be no longer
in the common vq structure.

So, I'd suggest something like following:

	if (is_vdev(..)) {
		/* comment here that we use VA for vdev */
		vq->vq_ring_mem = (phys_addr_t)vq->mz->addr;
		vq->virtio_net_hdr_mem = ...;
		vq->offset = ...;
	} else {
		vq->vq_ring_mem = ...;
		...
	}
	vring_hdr_desc_init(vq);

> See below comments for more detail.
> 
> >
> >The "setup_queue" method is actually for telling the device where desc,
> >avail and used vring are located. Hence, the implementation could be simple:
> >just log them.
> 
> >
> >>+
> >>+const struct virtio_pci_ops vdev_ops = {
> >Note that this is the interface for the driver to talk to the device,
> >we should put this file into upper layer then, in the driver.
> >
> >And let me make a summary, trying to make it clear:
> >
> >- We should not use any structures/functions from the virtio driver
> >   here, unless it's really a must.
> 
> Firstly I agree this point (although I see a difference in how we take "a
> must"). My original principle is to maximize the use of existing structures
> instead of maintain any new ones.

If that could save you a lot of efforts and make the design clean, I
might would say, yes, go for it. But it's obviously NO in this case.

> And I already give up that principle when
> I accept your previous suggestion to use struct virtio_user_device to store
> virtio-user specific fields. So I agree to add the new struct virtqueue to
> avoid use of driver-layer virtqueues.
> 
> >
> >- It's allowed for driver to make *few* special handling for the virtio
> >   user device. And that's what the driver supposed to do: to handle
> >   different device variants.
> 
> So here are two contradictory ways. Compared to the way you suggest,
> another way is to keep a unified driver and maintain all special handling
> inside struct virtio_pci_ops.
> 
> I prefer the latter because:
> (1) Special handling for each kind of device will be gather together instead
> of scattered everywhere of driver code.
> (2) It's more aligned to previous logic to hide the detail to differentiate
> modern/legacy device.

May I ask how many more such handling are needed, excluding the tx queue
header desc setup? And as stated, in generic, yes, we should try that.

	--yliu