From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.2393.1610504128317514855 for ; Tue, 12 Jan 2021 18:15:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PSpjcYmt; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610504127; 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=LtfSqN4cx9H+wFGcjW2TYrVuUXl7lYa/5SIHBNqY+hg=; b=PSpjcYmtVUSaWG6eZpJd969oIp7wwHhEzOTSRCV7Uv95LQUEIC9B/LcisEFHU8rfXVyaS+ 9w+5leq+vAJ8hm1tU6DHoo5B0gQh7Qf/BemwN63djVvSVu3W9/OQsDSGxb2acg8lWbFRMn wesVZGq7quHwgV9Ji5BbVh7RJCjcShk= 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-327-4oQDGHn3PH2jpvKoCwjXpw-1; Tue, 12 Jan 2021 21:15:21 -0500 X-MC-Unique: 4oQDGHn3PH2jpvKoCwjXpw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BE52A107ACF8; Wed, 13 Jan 2021 02:15:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-31.ams2.redhat.com [10.36.112.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8D7006A251; Wed, 13 Jan 2021 02:15:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() To: devel@edk2.groups.io, cenjiahui@huawei.com 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> From: "Laszlo Ersek" Message-ID: <47924678-b908-e946-6785-c07fd39f2c53@redhat.com> Date: Wed, 13 Jan 2021 03:15:15 +0100 MIME-Version: 1.0 In-Reply-To: <20210112094549.10238-6-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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/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", There are some other syntactic issues with this patch set that "PatchCheck.py" catches. 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); > } > > /** >