From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1A102A04DD; Thu, 22 Oct 2020 11:58:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E8954A998; Thu, 22 Oct 2020 11:58:11 +0200 (CEST) Received: from out0-139.mail.aliyun.com (out0-139.mail.aliyun.com [140.205.0.139]) by dpdk.org (Postfix) with ESMTP id A78AAA988 for ; Thu, 22 Oct 2020 11:58:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alibaba-inc.com; s=default; t=1603360685; h=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type; bh=cstadBD/0c7jpnULVq2h9P22ynHfCmOAW8O7h1KOdKI=; b=GE8/z88bfKLJXgmh83tBGm7qIQjYHicnJDF60wb6UYhMgyuMgqbBr6F3SHZT/TCOkUJleBYQ95dUuc4rcc+DNnb75MTLbG9+JO01LAiPFT4fWJsv+l/mf1Jx1A3Q8D2HPtknXV19z58t58Nu2R8WKsu7Riz/stS3isZofLmLAvA= X-Alimail-AntiSpam: AC=PASS; BC=-1|-1; BR=01201311R601e4; CH=green; DM=||false|; DS=||; FP=0|-1|-1|-1|0|-1|-1|-1; HT=ay29a033018047213; MF=huawei.xhw@alibaba-inc.com; NM=1; PH=DS; RN=8; SR=0; TI=SMTPD_---.Imt5BwZ_1603360683; Received: from 30.43.67.230(mailfrom:huawei.xhw@alibaba-inc.com fp:SMTPD_---.Imt5BwZ_1603360683) by smtp.aliyun-inc.com(127.0.0.1); Thu, 22 Oct 2020 17:58:04 +0800 To: Ferruh Yigit , Maxime Coquelin Cc: dev@dpdk.org, anatoly.burakov@intel.com, david.marchand@redhat.com, grive@u256.net, zhihong.wang@intel.com, chenbo.xia@intel.com References: <68ecd941-9c56-4de7-fae2-2ad15bdfd81a@alibaba-inc.com> <1602578499-13975-1-git-send-email-huawei.xhw@alibaba-inc.com> <1602578499-13975-2-git-send-email-huawei.xhw@alibaba-inc.com> <2298e6a6-2ca8-0f68-6742-1b7d5dde97a7@intel.com> <5d26db1a-6970-6e3a-c2e5-8dc540922028@alibaba-inc.com> From: "=?UTF-8?B?6LCi5Y2O5LyfKOatpOaXtuatpOWIu++8iQ==?=" Message-ID: <217038c8-7632-a8b0-95cd-c7f866735e6e@alibaba-inc.com> Date: Thu, 22 Oct 2020 17:57:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.3.3 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v4] pci: support both PIO and MMIO BAR for legacy virtio on x86 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2020/10/22 17:44, Ferruh Yigit wrote: > On 10/22/2020 10:15 AM, 谢华伟(此时此刻） wrote: >> >> On 2020/10/22 1:24, Ferruh Yigit wrote: >>> On 10/21/2020 1:32 PM, 谢华伟(此时此刻） wrote: >>>> >>>> On 2020/10/21 19:49, Ferruh Yigit wrote: >>>>> On 10/13/2020 9:41 AM, 谢华伟(此时此刻） wrote: >>>>>> From: "huawei.xhw" >>>>>> >>>>>> Legacy virtio-pci only supports PIO BAR resource. As we need to >>>>>> create lots of >>>>>> virtio devices and PIO resource on x86 is very limited, we expose >>>>>> MMIO BAR. >>>>>> >>>>>> Kernel supports both PIO  and MMIO BAR for legacy virtio-pci >>>>>> device. We handles >>>>>> different type of BAR in the similar way. >>>>>> >>>>>> In previous implementation, with igb_uio we get PIO address from >>>>>> igb_uio >>>>>> sysfs entry; with uio_pci_generic, we get PIO address from >>>>>> /proc/ioports. >>>>>> For PIO/MMIO RW, there is different path for different drivers >>>>>> and arch. >>>>>> For VFIO, PIO/MMIO RW is through syscall, which has big performance >>>>>> issue. >>>>>> On X86, it assumes only PIO is supported. >>>>>> >>>>>> All of the above is too much twisted. >>>>>> This patch unifies the way to get both PIO and MMIO address for >>>>>> different driver >>>>>> and arch, all from standard resource attr under pci sysfs. >>>>>> >>>>> >>>>> As mentined above this patch does multiple things. >>>>> >>>>> The main target is, as far as I understand, you have a legacy >>>>> virtio device which supports "memory-mapped I/O" and "port-mapped >>>>> I/O", but virtio logic forces legacy devices to use the PIO but >>>>> you want to be able to use the MMIO with this device. >>>> yes. >>>>> >>>>> The solution below is adding MMIO support in the PIO funciton, and >>>>> distinguish MMIO or PIO based on their address check. >>>> Yes, kernel does this in the similar way. >>>>> >>>>> >>>>> Instead of this, can't this be resolved in the virtio side, like >>>>> if the legacy device supports MMIO (detect this somehow) use the >>>>> MMIO istead of hacking PIO mapping to support MMIO? >>>> >>>> Get your concern. >>>> >>>> 1> >>>> >>>> If we move, I think we should move all those PCI codes into virtio >>>> side, not just the mmio part. >>>> >>>> Without my patch, those PCI codes are virtio-pci device specific, >>>> not generic. >>>> >>>> With this patch, those pci ioport map/rw code could also be used >>>> for other devices if they support both PIO and MMIO. >>>> >>> >>> I was not suggesting moving any code into virtio, but within >>> 'vtpci_init()' what happens when "hw->modern = 1;" is set? >>> And if this is set for your device, will it work without change? >> >> Yes, this will only affect legacy_device, which uses legacy_ops to >> access port io. >> >> If is is modern_device, port access will go through modern_ops. >> >> We only change the implementation in legacy_ops. >> > > I am saying something else. > > When a device is marked as "hw->modern = 1;", it will use MMIO, right? > If, somehow, your device marked as "hw->modern = 1;", will that path > work as expected for your device? modern device means virtio 1.0 and above. It has different register layout, so i couldn't mark legacy virtio device with MMIO as modern to make it work. Is this your question? /huawei > >> >>> >>>> Every option is ok. Hope i make myself clear. >>>> >>>> 2>  I don't think this is hacking. for >>>> rte_pci_ioport_map/read/write, if ioport could be both PIO and >>>> MMIO, then everything is reasonable. >>>> >>>> Take how kernel does port map for example: >>>> >>>>      vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0); >>>> >>>> Here io doesn't mean PIO only. It could also be MMIO. Kernel then >>>> uses ioread/write to access PIO/MMIO port. >>>> >>>> Actually we are pretty much the same in the interface. >>>> >>>> I think this patch extends rather then hacks the ioport interface >>>> to support MMIO. >>>> >>>>> >>>>> I have other concerns, specially mergin VFIO mapping too, but lets >>>>> clarify above first. >>>> >>>> vfio doesn't affect other driver but only virtio. >>>> >>> >>> Why it doesn't affect other drivers, can't there be other driver >>> using PIO? >> >> Currently only virtio-pci uses PIO, and only virtio PMD uses these >> port map/read/write functions. >> >> I don't foresee in future any new device uses PIO. >> > > I see but technically there can be other users. If there are other PIO users, what it matters is that we should keep these PCI port map/RW functions in pci layer rather than move to virtio PMD. Our patch only makes things better, as it supports both PIO and MMIO. > >> /huawei >> >>> >>>> igb_uio, uio_pci_generic and vfio-pci all uses the same way to >>>> map/rw ioport. >>>> >>> >>> For vfio, code changes 'pci_vfio_ioport_read()' to the direct >>> address read, first I don't know if this is always safe, and my >>> question why there is a syscall introduced at first place if you can >>> read from address directly? >> >> Original vfio way works, but we don't need that syscall. Under >> whatever driver, we could use the simple way as in this patch. >> > > If vfio works, you have already a solution, that is good. But I see > you are not happy with its performance. Different driver could go through the same code path. It doesn't need to be that complicated. Or we can say, this patch solves vfio performance issue in the mean time. > >> /huawei >> >>> >>> Is your device works as expected when vfio-pci kernel module used? >>> Since it is not suffering from PIO limitation, right? >> >> Certainly i tested vfio module. Firstly, i didn't intend to fix vfio >> performance issue, but i heard that igb_uio will be removed. >> > > Yes, it will be removed in the long run. > >> /huawei >> >>> >>> >>> And I wonder if the patch can be done as three patches to simply it, >>> as: >>> 1) Combine 'RTE_PCI_KDRV_IGB_UIO' & 'RTE_PCI_KDRV_UIO_GENERIC' >>> (remove pci_ioport_map) >>> 2) Update 'pci_uio_ioport_map()' to add memory map support (and >>> update read/write functions according) >>> 3) Combine vfio & uio >>> >> Got it. It makes sense to split, but i think this patch is already >> simple enough. >> > > The patch is doing many things in one patch, I think it is better to > separate logically separate issues, although they are simple. Splitting. > >> Let me check. >> >> /huawei >> >>>>> >>>>> Thanks, >>>>> ferruh >>>>> >>>>> >>>>> >>>>>> We distinguish PIO and MMIO by their address like how kernel >>>>>> does. It is ugly but works. >>>>>> >>>>>> Signed-off-by: huawei.xhw >>> >>> <...>