From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mx.groups.io with SMTP id smtpd.web12.10852.1607064531514638779 for ; Thu, 03 Dec 2020 22:48:52 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.190, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS412-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4CnNZP15xLz15XH7; Fri, 4 Dec 2020 14:48:13 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS412-HUB.china.huawei.com (10.3.19.212) with Microsoft SMTP Server id 14.3.487.0; Fri, 4 Dec 2020 14:48:34 +0800 Subject: Re: [edk2-devel] [PATCH v2 0/4] Add extra pci roots support for Arm To: , CC: , , , , , References: <20201109130511.5946-1-cenjiahui@huawei.com> From: "Jiahui Cen" Message-ID: <5d2daf54-a57b-c4bd-a757-cf6b710cd4e5@huawei.com> Date: Fri, 4 Dec 2020 14:48:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.174.184.155] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, During the modification of this patch series, I got a confusing problem that the EDK2's Alignment seems not fit with Linux on ARM. EDK2, in CalculateResourceAperture, aligns the offset of child nodes in descending order and uses the *Bridge's alignment* to align the total length of Bridge. However, Linux, in pbus_size_mem, would align the size of child nodes and use *the half of max alignment of child nodes* to align the total length of Bridge. eg. A Root Bridge with Bus [d2] -+-[0000:d2]---01.0-[d3]----01.0 where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) [size=256] [d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) [size=128K] BAR4 (mem, 64-bit, pref) [size=64M] In EDK2, the Resource Map would be: PciBus: Resource Map for Root Bridge PciRoot(0xD2) Type = Mem64; Base = 0x8004000000; Length = 0x4200000; Alignment = 0x3FFFFFF Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF; Owner = PPB [D2|01|00:**]; Type = PMem64 Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF; Owner = PPB [D2|01|00:10] PciBus: Resource Map for Bridge [D2|01|00] Type = PMem64; Base = 0x8004000000; Length = 0x4100000; Alignment = 0x3FFFFFF Base = 0x8004000000; Length = 0x4000000; Alignment = 0x3FFFFFF; Owner = PCI [D3|01|00:20] Base = 0x8008000000; Length = 0x20000; Alignment = 0x1FFFF; Owner = PCI [D3|01|00:10] Type = Mem64; Base = 0x8008100000; Length = 0x100; Alignment = 0xFFF It shows that with EDK2's alignment [d2:01.00] only requires a PMem64 resource range of 0x4100000. However in Linux, [d2:01.00]'s BAR14 (Prefetchable Memory Resource behind the Bridge) requires a PMem64 resource range of 0x06000000, which comes from (0x4000000 + 0x20000) with alignment=0x1FFFFFF the half of the max alignment 0x3FFFFFF. Therefore, the assignment for [d2:01.00]'s BAR14 will fail. The difference could make the resource range allocated in EDK2 smaller than the resource range needed in Linux, which causes the assignment failure in Linux. The same difference also occurs when io resource allocation. To handle this senario, is it necessary to calculate the resource map in Linux way? Furthermore, Linux Kernel will treat pcie-root-port as a hotplugable bridge and try to require more resource. But EDK2 seems not support hotplug padding for ARM? Thanks, Jiahui On 2020/11/11 22:33, Laszlo Ersek wrote: > On 11/09/20 14:05, Jiahui Cen wrote: >> Changes with v1 >> v1->v2: >> Separated into four patches. >> Factor the same logic parts into a new library. >> >> v1: https://edk2.groups.io/g/devel/topic/72723351#56901 >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 > > (1) Each commit message in the series should reference this bugzilla. > > But, in itself, that's no reason for a repost; such an update can be > made by maintainers when they merge the series. > > (2) This is a feature addition, so it's merge material for the next > development cycle > . > (Also it depends on the QEMU work being merged first.) I'm going to > proceed with the review now. > > Thanks, > Laszlo > >> QEMU: https://lore.kernel.org/qemu-devel/20201103120157.2286-1-cenjiahui@huawei.com/ >> >> This patch series adds support for extra pci roots for ARM. >> >> In order to avoid duplicated codes, we introduce a new library >> PciHostBridgeUtilityLib which extracts common interfaces from >> OvmfPkg/PciHostBridgeLib. It provides conflicts informing and extra pci >> roots scanning. Using the utility lib, the uefi could scan for extra >> root buses and recognize multiple roots for ARM. >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Leif Lindholm >> Signed-off-by: Yubo Miao >> Signed-off-by: Jiahui Cen >> >> Yubo Miao (4): >> OvmfPkg: Extract functions form PciHostBridgeLib >> ArmVirtPkg: Use extracted PciHostBridgeUtilityLib >> OvmfPkg: Extract functions of extra pci roots >> ArmVirtPkg: Support extra pci roots >> >> ArmVirtPkg/ArmVirt.dsc.inc | 1 + >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> OvmfPkg/OvmfXen.dsc | 1 + >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 5 + >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 + >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 51 ++++ >> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 98 +++++++ >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 221 ++++++++------- >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 234 +--------------- >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 283 ++++++++++++++++++++ >> 12 files changed, 563 insertions(+), 335 deletions(-) >> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> create mode 100644 OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h >> create mode 100644 OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c >> > > > > > > > . >