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

[Tan, Jianfeng]

On 5/13/2016 12:40 AM, Yuanhan Liu wrote:
> 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.

I agree about this point. But is it really big deal? Please go on with 
remaining comments before make decision.

> BTW, it's an implicit check, which hurts readability a bit.

At the point of readability, I think driver does not need to care about 
devices use physical addresses or virtual addresses, and this is why we 
do that inside pci ops to hide those details.

Another side, pci ops is introduced to cover the details of talking to a 
device as I understand, do you think it offends such semantics? (this 
one could be your argument :-))

>
> 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

Each kind of device does such check/settings on its own requirement. So 
only indispensable check/settings are added into these three different 
devices.

>
> 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.

Yes, this is a blocker issue of my current implementation. Let's see you 
suggestion firstly.

>
> So, I'd suggest something like following:
>
> 	if (is_vdev(..)) {

The blocker issue of your suggestion is that we have no such condition.

Previously, I use dev_type, but as David's comment said:

    "The reason of those comments is that dev_type in ethdev is going to
    disappear, see [1] and [2].
    Drivers are called through their own specific ethdev/crypto ops and
    so, those drivers know implicitely that their are either pci or vdev
    (or whatever in the future) drivers."

Another consideration is comparing vtpci_ops pointer, I don't think it's 
elegant.

> 		/* 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.

Those which need special handling:
(1) vq->vq_ring_mem: it is set but never used, so it's out of question.
(2) vq->virtio_net_hdr_mem and vring_hdr_desc_init
(3) vq->offset

Just (2) and (3) so far. And the question is quite clear: where to put 
these two special handling.

Thanks,
Jianfeng

>
> 	--yliu