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.web11.4470.1610518246967971242 for ; Tue, 12 Jan 2021 22:10:47 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.190, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4DFxq86dHxzl4M3; Wed, 13 Jan 2021 14:09:24 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Wed, 13 Jan 2021 14:10:33 +0800 Subject: Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() To: Laszlo Ersek , CC: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , "Julien Grall" , Leif Lindholm , Sami Mujawar , , , "Yubo Miao" References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-6-cenjiahui@huawei.com> <47924678-b908-e946-6785-c07fd39f2c53@redhat.com> From: "Jiahui Cen" Message-ID: Date: Wed, 13 Jan 2021 14:10:31 +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: <47924678-b908-e946-6785-c07fd39f2c53@redhat.com> 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, On 2021/1/13 10:15, Laszlo Ersek wrote: > On 01/12/21 10:45, Jiahui Cen via groups.io wrote: >> Rebase ArmVirtPkg/FdtPciHostBridgeLib to the new >> PciHostBridgeUtilityInitRootBridge() / PciHostBridgeUtilityUninitRootBridge() >> functions. >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 >> >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Leif Lindholm >> Signed-off-by: Jiahui Cen >> Signed-off-by: Yubo Miao >> --- >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 1 + >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 124 ++++++++++---------- >> 2 files changed, 61 insertions(+), 64 deletions(-) >> >> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf >> index 01d39626d14c..b813a0851d2a 100644 >> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf >> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf >> @@ -34,6 +34,7 @@ [Packages] >> OvmfPkg/OvmfPkg.dec >> >> [LibraryClasses] >> + BaseMemoryLib >> DebugLib >> DevicePathLib >> DxeServicesTableLib >> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> index d554479bf0de..95166d18c82d 100644 >> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> @@ -7,6 +7,7 @@ >> >> **/ >> #include >> +#include >> #include >> #include >> #include >> @@ -20,37 +21,6 @@ >> #include >> #include >> >> -#pragma pack(1) >> -typedef struct { >> - ACPI_HID_DEVICE_PATH AcpiDevicePath; >> - EFI_DEVICE_PATH_PROTOCOL EndDevicePath; >> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH; >> -#pragma pack () >> - >> -STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = { >> - { >> - { >> - ACPI_DEVICE_PATH, >> - ACPI_DP, >> - { >> - (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)), >> - (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8) >> - } >> - }, >> - EISA_PNP_ID(0x0A03), >> - 0 >> - }, >> - >> - { >> - END_DEVICE_PATH_TYPE, >> - END_ENTIRE_DEVICE_PATH_SUBTYPE, >> - { >> - END_DEVICE_PATH_LENGTH, >> - 0 >> - } >> - } >> -}; >> - >> // >> // We expect the "ranges" property of "pci-host-ecam-generic" to consist of >> // records like this. >> @@ -319,11 +289,18 @@ PciHostBridgeGetRootBridges ( >> UINTN *Count >> ) >> { >> - UINT64 IoBase, IoSize; >> - UINT64 Mmio32Base, Mmio32Size; >> - UINT64 Mmio64Base, Mmio64Size; >> - UINT32 BusMin, BusMax; >> - EFI_STATUS Status; >> + UINT64 IoBase, IoSize; >> + UINT64 Mmio32Base, Mmio32Size; >> + UINT64 Mmio64Base, Mmio64Size; >> + UINT32 BusMin, BusMax; >> + EFI_STATUS Status; >> + UINT64 Attributes; >> + UINT64 AllocationAttributes; >> + PCI_ROOT_BRIDGE_APERTURE Io; >> + PCI_ROOT_BRIDGE_APERTURE Mem; >> + PCI_ROOT_BRIDGE_APERTURE MemAbove4G; >> + PCI_ROOT_BRIDGE_APERTURE PMem; >> + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; >> >> if (PcdGet64 (PcdPciExpressBaseAddress) == 0) { >> DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__)); >> @@ -341,33 +318,29 @@ PciHostBridgeGetRootBridges ( >> return NULL; >> } >> >> - *Count = 1; >> + ZeroMem (&Io, sizeof (Io)); >> + ZeroMem (&Mem, sizeof (Mem)); >> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G)); >> + ZeroMem (&PMem, sizeof (PMem)); >> + ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G)); >> >> - mRootBridge.Segment = 0; >> - mRootBridge.Supports = EFI_PCI_ATTRIBUTE_ISA_IO_16 | >> - EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | >> - EFI_PCI_ATTRIBUTE_VGA_IO_16 | >> - EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16; >> - mRootBridge.Attributes = mRootBridge.Supports; >> + Attributes = EFI_PCI_ATTRIBUTE_ISA_IO_16 | >> + EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | >> + EFI_PCI_ATTRIBUTE_VGA_IO_16 | >> + EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16; >> >> - mRootBridge.DmaAbove4G = TRUE; >> - mRootBridge.NoExtendedConfigSpace = FALSE; >> - mRootBridge.ResourceAssigned = FALSE; >> + AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM; >> >> - mRootBridge.AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM; >> - >> - mRootBridge.Bus.Base = BusMin; >> - mRootBridge.Bus.Limit = BusMax; >> - mRootBridge.Io.Base = IoBase; >> - mRootBridge.Io.Limit = IoBase + IoSize - 1; >> - mRootBridge.Mem.Base = Mmio32Base; >> - mRootBridge.Mem.Limit = Mmio32Base + Mmio32Size - 1; >> + Io.Base = IoBase; >> + Io.Limit = IoBase + IoSize - 1; >> + Mem.Base = Mmio32Base; >> + Mem.Limit = Mmio32Base + Mmio32Size - 1; >> >> if (sizeof (UINTN) == sizeof (UINT64)) { >> - mRootBridge.MemAbove4G.Base = Mmio64Base; >> - mRootBridge.MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1; >> + MemAbove4G.Base = Mmio64Base; >> + MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1; >> if (Mmio64Size > 0) { >> - mRootBridge.AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE; >> + AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE; >> } >> } else { >> // >> @@ -376,19 +349,41 @@ PciHostBridgeGetRootBridges ( >> // BARs unless they are allocated below 4 GB. So ignore the range above >> // 4 GB in this case. >> // >> - mRootBridge.MemAbove4G.Base = MAX_UINT64; >> - mRootBridge.MemAbove4G.Limit = 0; >> + MemAbove4G.Base = MAX_UINT64; >> + MemAbove4G.Limit = 0; >> } >> >> // >> // No separate ranges for prefetchable and non-prefetchable BARs >> // >> - mRootBridge.PMem.Base = MAX_UINT64; >> - mRootBridge.PMem.Limit = 0; >> - mRootBridge.PMemAbove4G.Base = MAX_UINT64; >> - mRootBridge.PMemAbove4G.Limit = 0; >> + PMem.Base = MAX_UINT64; >> + PMem.Limit = 0; >> + PMemAbove4G.Base = MAX_UINT64; >> + PMemAbove4G.Limit = 0; >> >> - mRootBridge.DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath; >> + Status = PciHostBridgeUtilityInitRootBridge ( >> + Attributes, >> + Attributes, >> + AllocationAttributes, >> + TRUE, >> + FALSE, >> + BusMin, >> + BusMax, >> + &Io, >> + &Mem, >> + &MemAbove4G, >> + &PMem, >> + &PMemAbove4G, >> + &mRootBridge >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n", >> + __FUNCTION__, Status)); > > (1) EFI_D_ERROR will fail "BaseTools/Scripts/PatchCheck.py": > >> Code format is not valid: >> * EFI_D_ERROR was used, but DEBUG_ERROR is now recommended >> File: ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> Line: DEBUG ((EFI_D_ERROR, "%a: failed to initialize PCI host bridge: %r\n", > Sorry for forgetting to check patches. Will fix it. BTW, EFI_D_ERROR are also used in the other part of the same function, do I need to fix them too? > There are some other syntactic issues with this patch set that > "PatchCheck.py" catches. Another issue is that the subject is too long. Will try to shorten it. Seems patch [8/9] has the same issue, will fix too. Thanks, Jiahui > > It's simplest if you open a pull request with your topic branch, about > to be submitted to the list, on github.com. The PR will be auto-closed, > but it will run CI on your patch series first, and you will get a chance > to fix issues before the on-list review. The CI tasks include > "PatchCheck". > > You can also run "BaseTools/Scripts/PatchCheck.py" locally. (You can run > more or less the whole CI content locally, but setting that up is not > quick.) > > With the PatchCheck errors addressed, this patch will be OK. > > Thanks, > Laszlo > > >> + *Count = 0; >> + return NULL; >> + } >> + >> + *Count = 1; >> >> return &mRootBridge; >> } >> @@ -408,6 +403,7 @@ PciHostBridgeFreeRootBridges ( >> ) >> { >> ASSERT (Count == 1); >> + PciHostBridgeUtilityUninitRootBridge (Bridges); >> } >> >> /** >> > > . >