From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id E14AC8E5D for ; Thu, 14 Jan 2016 06:58:33 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 13 Jan 2016 21:58:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,293,1449561600"; d="scan'208";a="890186208" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga002.jf.intel.com with ESMTP; 13 Jan 2016 21:58:31 -0800 Date: Thu, 14 Jan 2016 13:59:49 +0800 From: Yuanhan Liu To: Tetsuya Mukawa Message-ID: <20160114055949.GJ19531@yliu-dev.sh.intel.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <569723B9.5080904@igel.co.jp> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org, "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 05:58:34 -0000 On Thu, Jan 14, 2016 at 01:27:37PM +0900, Tetsuya Mukawa wrote: > On 2016/01/12 15:58, Yuanhan Liu wrote: > > v2: - fix a data corruption reported by Qian, due to hdr size mismatch. > > check detailes at ptach 5. > > > > - Add missing config_irq and isr reading support from v1. > > > > - fix comments from v1. > > > > Almost all difference comes from virtio 1.0 are the PCI layout change: > > the major configuration structures are stored at bar space, and their > > location is stored at corresponding pci cap structure. Reading/parsing > > them is one of the major work of patch 7. > > > > To make handling virtio v1.0 and v0.95 co-exist well, this patch set > > introduces a virtio_pci_ops structure, to add another layer so that > > we could keep those vtpci_foo_bar "APIs". With that, we could do the > > minimum change to add virtio 1.0 support. > > > > > > 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 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? Jianfeng had same suggestion. I could do that in next version then. > 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. I don't think we can/need abstract virtio_read_caps() here. As that detects wheter it is a legacy or modern (virtio 1.0) virtio device or not. If virtio_read_caps failes, which could either due to pci map failed, or because malformed pci layout, we fallback to legacy virtio 1.0 handling, using io port read/write to do configuration. > 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. Actually, I didn't introduce the abstraction for pci device access. It's just a simple "if ... else ..." case here: use io port read/write, the VIRTIO_READ/WRITE_REG_X macros, to do access for legacy virtio, otherwise for modern virtio, use direct mapped memory read/write access: modern_read/writex. > Is it possible to separate? As stated, there is no mix, therefore no need for seperation. But you could add another access abstraction layer, and assign it properly later, say, by checking eth_dev->dev_type as you suggested below. > 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. It's abstracted by virtio_pci_ops. > - how to mmap PCI configuration space. > - how to read/(write) PCI configuration space. For now, it's actually done by EAL, or by functions provided by EAL. I haven't read your (as well Jianfeng's) code yet, but seems that you need implement another set of functions for above needes for your virtio device. If so, I'd suggest you (or Jianfeng) to do the abstraction based on my patchset: what I am kind of sure is that I should not add those abstraction here, simply for it has nothing to do with virtio 1.0 enabling. --yliu