From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3CA718DB3 for ; Thu, 14 Jan 2016 07:09:24 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 13 Jan 2016 22:09:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,293,1449561600"; d="scan'208";a="881062122" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.119]) ([10.239.67.119]) by fmsmga001.fm.intel.com with ESMTP; 13 Jan 2016 22:09:19 -0800 To: Tetsuya Mukawa , Yuanhan Liu , dev@dpdk.org References: <1449719650-3482-1-git-send-email-yuanhan.liu@linux.intel.com> <1452581944-24838-1-git-send-email-yuanhan.liu@linux.intel.com> <569723B9.5080904@igel.co.jp> From: "Tan, Jianfeng" Message-ID: <56973B8E.6010808@intel.com> Date: Thu, 14 Jan 2016 14:09:18 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <569723B9.5080904@igel.co.jp> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "Michael S. Tsirkin" Subject: Re: [dpdk-dev] [PATCH v2 0/7] virtio 1.0 enabling for virtio pmd driver 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, 14 Jan 2016 06:09:24 -0000 Hi Tetsuya, On 1/14/2016 12:27 PM, Tetsuya Mukawa wrote: > On 2016/01/12 15:58, Yuanhan Liu wrote: > Hi Yuanhan and Jianfeng, > > Thanks for great patches. > I want to use VIRTIO-1.0 feature for my virtio container patch, because > it will solve 44 bit memory address limitation. > (So far, legacy virtio-net device only receives queue address under (1 > << (32 + 12)).) I suppose you are specifying the code below: /* * Virtio PCI device VIRTIO_PCI_QUEUE_PF register is 32bit, * and only accepts 32 bit page frame number. * Check if the allocated physical memory exceeds 16TB. */ if ((mz->phys_addr + vq->vq_ring_size - 1) >> (VIRTIO_PCI_QUEUE_ADDR_SHIFT + 32)) { PMD_INIT_LOG(ERR, "vring address shouldn't be above 16TB!"); rte_free(vq); return -ENOMEM; } So you don't need to add extra cmd option, right? > > I have a few comments to rebase virtio container patches on this patches. > > 1. VIRTIO_READ_REG_X > > So far, VIRTIO_READ_REG_1/2/4 are defined in virtio_pci.h. > But these macros are only referred by virtio_pci.c. > How about moving the macros to virtio_pci.c? +1 for this. > > 2. Abstraction of read/write accesses. > > It may be difficult to cleanly rebase my patches on this patches, > because virtio_read_caps() is not abstracted. > Let me describe it more. > So far, we need to handle below 3 virtio-net devices.. > - physical virtio-net device. > - virtual virtio-net device in virtio-net PMD. (Jianfeng's patch) > - virtual virtio-net device in QEMU. (my patch) > > Almost all code of the virtio-net PMD can be shared between above > different cases. > Probably big difference is how to access to configuration space. > > Yuanhan's patch introduces an abstraction layer to hide configuration > space layout and how to access it. > Is it possible to separate? > I guess "access method" will be nice to be abstracted separately from > "configuration space layout". > Probably access method will be defined by "eth_dev->dev_type" and the > PMD name like "eth_cvio". > And "configuration space layout" will be defined by capability list of > PCI configuration layout. > > For example, if access method like below are abstracted separately and > current "virtio_pci.c" is implemented on this abstraction, we can easily > re-use virtio_read_caps(). > - how to read/write virtio configuration space. > - how to mmap PCI configuration space. > - how to read/(write) PCI configuration space. I basically agree with you. We have two dimensions here: legacy modern physical virtio device: Use virtio_read_caps_phys() to distinguish virtual virtio device (Tetsuya): Use virtio_read_caps_virt() to distinguish virtual virtio device (Jianfeng): does not need a "configuration space layout", no need to distinguish So in vtpci_init(), we needs to test "eth_dev->dev_type" firstly vtpci_init() { if (eth_dev->dev_type == RTE_ETH_DEV_PCI) { if (virtio_read_caps_phys()) { // modern } else { // legacy } } else { if (Tetsuya's way) { if (virtio_read_caps_virt()) { // modern } else { // legacy } } else { // Jianfeng's way } } } And from Yuanhan's angle, I think he does not need to address this problem. How do you think? Thanks, Jianfeng > > Thanks, > Tetsuya