From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.5580.1610528728316320173 for ; Wed, 13 Jan 2021 01:05:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZDPxTwhn; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610528727; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=1wDSO9z1jlsw7tsFB1eC8OR8dn03v9GfpL3Rm4VoFZg=; b=ZDPxTwhncV7rn+XR7ggDw7rfR2JjBz63clsGLM180d9D3koozfP6g+J99+t9XnYATcRHdo 2bFn7BD8PGXTWifitfDg/yuy8dx+uohmGXURz9ZCxPh52DwUadhUKfuGgggbJBaAQjsSkF h9PHN5pSlnBzb918yVG6cNKWkpaUiSk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-162-SopR9OE0O9aSSD9V6rPQng-1; Wed, 13 Jan 2021 04:05:23 -0500 X-MC-Unique: SopR9OE0O9aSSD9V6rPQng-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 65E2510766BA; Wed, 13 Jan 2021 09:05:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-238.ams2.redhat.com [10.36.112.238]) by smtp.corp.redhat.com (Postfix) with ESMTP id E3B816A908; Wed, 13 Jan 2021 09:05:17 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() To: Jiahui Cen , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , Julien Grall , Leif Lindholm , Sami Mujawar , xieyingtai@huawei.com, wu.wubin@huawei.com, Yubo Miao References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-6-cenjiahui@huawei.com> <47924678-b908-e946-6785-c07fd39f2c53@redhat.com> From: "Laszlo Ersek" Message-ID: <1d9dfe70-21d3-54a0-f180-eaa77a2643d0@redhat.com> Date: Wed, 13 Jan 2021 10:05:16 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/13/21 07:10, Jiahui Cen wrote: > 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? No, just please don't introduce new instances of EFI_D_*. Thanks, Laszlo > >> 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); >>> } >>> >>> /** >>> >> >> . >> >