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

[Yuanhan Liu]

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.

> +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? If it needs a special
handling, do it in driver.

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.

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