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

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

Hi yuanhan,


On 5/12/2016 10:12 AM, Yuanhan Liu wrote:
> On Fri, Apr 29, 2016 at 01:18:34AM +0000, Jianfeng Tan wrote:
>> +static void
>> +vdev_read_dev_config(struct virtio_hw *hw, uint64_t offset,
>> +		     void *dst, int length)
>> +{
>> +	int i;
>> +	struct virtio_user_hw *uhw = (struct virtio_user_hw *)hw->vdev_private;
> Unnecessary cast.

Yes.

>
>> +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, such as real virtio device needs to check 
address (patch 1). 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. 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.


Thanks,
Jianfeng


>
>    So, I think it's okay to export the virtio_user_device struct to
>    driver and do all those kind of "fake pci" configration there.
>
> 	--yliu
>
>> +	.read_dev_cfg	= vdev_read_dev_config,
>> +	.write_dev_cfg	= vdev_write_dev_config,
>> +	.reset		= vdev_reset,
>> +	.get_status	= vdev_get_status,
>> +	.set_status	= vdev_set_status,
>> +	.get_features	= vdev_get_features,
>> +	.set_features	= vdev_set_features,
>> +	.get_isr	= vdev_get_isr,
>> +	.set_config_irq	= vdev_set_config_irq,
>> +	.get_queue_num	= vdev_get_queue_num,
>> +	.setup_queue	= vdev_setup_queue,
>> +	.del_queue	= vdev_del_queue,
>> +	.notify_queue	= vdev_notify_queue,
>> +};
>> -- 
>> 2.1.4