From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 4C0F5689B for ; Thu, 12 May 2016 09:08:09 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP; 12 May 2016 00:08:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,609,1455004800"; d="scan'208";a="804536349" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by orsmga003.jf.intel.com with ESMTP; 12 May 2016 00:08:06 -0700 To: Yuanhan Liu References: <1461892716-19122-1-git-send-email-jianfeng.tan@intel.com> <1461892716-19122-7-git-send-email-jianfeng.tan@intel.com> <20160512021208.GA17474@yliu-dev.sh.intel.com> 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 From: "Tan, Jianfeng" Message-ID: Date: Thu, 12 May 2016 15:08:05 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160512021208.GA17474@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 6/8] virtio-user: add new virtual pci driver for virtio X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2016 07:08:09 -0000 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