From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH v4 6/8] virtio-user: add new virtual pci driver for virtio Date: Wed, 11 May 2016 19:12:08 -0700 Message-ID: <20160512021208.GA17474@yliu-dev.sh.intel.com> References: <1461892716-19122-1-git-send-email-jianfeng.tan@intel.com> <1461892716-19122-7-git-send-email-jianfeng.tan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, Huawei Xie , rich.lane@bigswitch.com, mst@redhat.com, nakajima.yoshihiro@lab.ntt.co.jp, p.fedin@samsung.com, ann.zhuangyanying@huawei.com, mukawa@igel.co.jp, nhorman@tuxdriver.com To: Jianfeng Tan Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id AF01B68F5 for ; Thu, 12 May 2016 04:07:12 +0200 (CEST) Content-Disposition: inline In-Reply-To: <1461892716-19122-7-git-send-email-jianfeng.tan@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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