DPDK patches and discussions
 help / color / mirror / Atom feed
From: "谢华伟(此时此刻)" <huawei.xhw@alibaba-inc.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: dev@dpdk.org, anatoly.burakov@intel.com,
	david.marchand@redhat.com, grive@u256.net,
	zhihong.wang@intel.com, chenbo.xia@intel.com
Subject: Re: [dpdk-dev] [PATCH v4] pci: support both PIO and MMIO BAR for legacy virtio on x86
Date: Thu, 22 Oct 2020 17:57:38 +0800	[thread overview]
Message-ID: <217038c8-7632-a8b0-95cd-c7f866735e6e@alibaba-inc.com> (raw)
In-Reply-To: <a6998c46-8026-7aec-0fe6-d96a05cb6a41@intel.com>


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" <huawei.xhw@alibaba-inc.com>
>>>>>>
>>>>>> 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 <huawei.xhw@alibaba-inc.com>
>>>
>>> <...>

  reply	other threads:[~2020-10-22  9:58 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-30 14:59 [dpdk-dev] [PATCH v2] " 谢华伟(此时此刻)
2020-10-01 10:22 ` Burakov, Anatoly
2020-10-02  5:44   ` 谢华伟(此时此刻)
2020-10-09  8:36 ` [dpdk-dev] [PATCH v3] " 谢华伟(此时此刻)
2020-10-13  8:41 ` [dpdk-dev] [PATCH v4] support both PIO and MMIO bar for virtio pci device 谢华伟(此时此刻)
2020-10-13  8:41   ` [dpdk-dev] [PATCH v4] pci: support both PIO and MMIO BAR for legacy virtio on x86 谢华伟(此时此刻)
2020-10-13 12:34     ` 谢华伟(此时此刻)
2020-10-21  8:46     ` 谢华伟(此时此刻)
2020-10-21 11:49     ` Ferruh Yigit
2020-10-21 12:32       ` 谢华伟(此时此刻)
2020-10-21 17:24         ` Ferruh Yigit
2020-10-22  9:15           ` 谢华伟(此时此刻)
2020-10-22  9:44             ` Ferruh Yigit
2020-10-22  9:57               ` 谢华伟(此时此刻) [this message]
2020-10-22 15:51 ` [dpdk-dev] [PATCH v5 0/3] support both PIO and MMIO BAR for virtio PMD 谢华伟(此时此刻)
2020-10-22 15:51   ` [dpdk-dev] [PATCH v5 1/3] PCI: use PCI standard sysfs entry to get PIO address 谢华伟(此时此刻)
2021-01-12  8:07     ` Maxime Coquelin
2021-01-14 18:23       ` 谢华伟(此时此刻)
2021-01-24 15:10         ` Xueming(Steven) Li
2020-10-22 15:51   ` [dpdk-dev] [PATCH v5 2/3] PCI: support MMIO in rte_pci_ioport_map/unap/read/write 谢华伟(此时此刻)
2021-01-12  8:23     ` Maxime Coquelin
2021-01-21  6:30       ` 谢华伟(此时此刻)
2021-01-24 15:22     ` Xueming(Steven) Li
2021-01-25  3:08       ` 谢华伟(此时此刻)
2021-01-27 10:40     ` Ferruh Yigit
2021-01-27 15:34       ` 谢华伟(此时此刻)
2021-01-27 16:45         ` Ferruh Yigit
2020-10-22 15:51   ` [dpdk-dev] [PATCH v5 3/3] PCI: don't use vfio ioctl call to access PIO resource 谢华伟(此时此刻)
2021-01-12  9:37     ` Maxime Coquelin
2021-01-12 16:58       ` Maxime Coquelin
2021-01-20 14:54         ` 谢华伟(此时此刻)
2021-01-21  8:29           ` Maxime Coquelin
2021-01-21 14:57             ` 谢华伟(此时此刻)
2021-01-21 15:00               ` 谢华伟(此时此刻)
2021-01-21 15:38               ` Maxime Coquelin
2021-01-22  7:25                 ` 谢华伟(此时此刻)
2021-01-26 10:44                   ` Maxime Coquelin
2021-01-27 10:32                     ` Ferruh Yigit
2021-01-27 12:17                       ` Maxime Coquelin
2021-01-27 14:43                       ` 谢华伟(此时此刻)
2021-01-27 16:45                         ` Ferruh Yigit
2021-01-28 13:43                           ` 谢华伟(此时此刻)
2021-01-26 12:30                   ` 谢华伟(此时此刻)
2021-01-26 12:35                     ` Maxime Coquelin
2021-01-26 14:24                       ` 谢华伟(此时此刻)
2020-10-27  8:50   ` [dpdk-dev] [PATCH v5 0/3] support both PIO and MMIO BAR for virtio PMD 谢华伟(此时此刻)
2020-10-28  3:48     ` 谢华伟(此时此刻)
2020-11-02 11:56   ` 谢华伟(此时此刻)
2020-11-10 12:35   ` 谢华伟(此时此刻)
2020-11-10 12:42     ` David Marchand
2020-11-12 13:35       ` 谢华伟(此时此刻)
2020-12-14 14:24       ` 谢华伟(此时此刻)
2020-12-16  7:54         ` Maxime Coquelin
2021-01-12 17:37   ` Maxime Coquelin
2021-01-14 18:19     ` 谢华伟(此时此刻)
2021-01-21  4:12     ` 谢华伟(此时此刻)
2021-01-21  8:47       ` Maxime Coquelin
2021-01-21 13:51         ` 谢华伟(此时此刻)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=217038c8-7632-a8b0-95cd-c7f866735e6e@alibaba-inc.com \
    --to=huawei.xhw@alibaba-inc.com \
    --cc=anatoly.burakov@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=grive@u256.net \
    --cc=maxime.coquelin@redhat.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).