From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x235.google.com (mail-it0-x235.google.com [IPv6:2607:f8b0:4001:c0b::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D754D1A1E2B for ; Wed, 24 Aug 2016 08:19:24 -0700 (PDT) Received: by mail-it0-x235.google.com with SMTP id f6so39319244ith.0 for ; Wed, 24 Aug 2016 08:19:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=bj5qhLuLZ2V5Qpf6hwp5Tm1GXmo1aosXaHlHuTjsdg8=; b=jStONd6d4uID+SZhuNgtLjHeaMnH4+9FjhjIDJuENPq6+AVdZvXPdiPWqvK5OQWH6T zV1M8I/9FDzh1XrlQKMuKRAIcnetNNjzTbGokI3FtepM7KmG7gSUpLyx9aLeecl42nQ8 cOA0EBRfiY0LIm/1QohrqB/ZCuCCWPNc6KWzc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=bj5qhLuLZ2V5Qpf6hwp5Tm1GXmo1aosXaHlHuTjsdg8=; b=K0pmV9ROC3C1jVT2bE6XKfZJmeCulDF1ZRuZw0gzCcp7T0oms8DdMwv5GR8ThHycsN O2KUldO1cfLm6QgOJzj1sPy7SyYoR2+SwzW5BcKx5K7iOKevfieiwrSPDkYouw9HMzRx l6bqEv8f+oLGHmZjuCkdwyag6SuoOYGRraFEAwihi+PTTL5F2T3iTz9ZXpksTDq+wUv5 EYe6Z4+uI5lnirVJrwc87ASmuGdQ9RNOqN5kWCIki5AUueE78XeG1158i+pQJ/XIwfcv xU9N57QgdW1lflMW13Bfpgx8V/2bAwokORT3hccBWCUWAwgEeXud0m7s6rWXqHs1AOBr 65fw== X-Gm-Message-State: AEkoouvlX0djQn4ZH3Y34UQdjwcooaaa8ERBDLI3EFX1sDlj25M1m2CuqomSsz4ck1IbOVXQy2e0RPJOoamREIzi X-Received: by 10.36.214.193 with SMTP id o184mr5078530itg.5.1472051963401; Wed, 24 Aug 2016 08:19:23 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Wed, 24 Aug 2016 08:19:22 -0700 (PDT) In-Reply-To: <8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com> References: <1471847752-26574-1-git-send-email-ard.biesheuvel@linaro.org> <1471847752-26574-3-git-send-email-ard.biesheuvel@linaro.org> <8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com> From: Ard Biesheuvel Date: Wed, 24 Aug 2016 17:19:22 +0200 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Aug 2016 15:19:25 -0000 Content-Type: text/plain; charset=UTF-8 On 24 August 2016 at 17:01, Laszlo Ersek wrote: > On 08/22/16 02:35, Ard Biesheuvel wrote: >> Implement PciHostBridgeLib for DT platforms that expose a PCI root bridge >> via a pci-host-ecam-generic DT node. The DT parsing logic is copied from >> the PciHostBridgeDxe implementation in ArmVirtPkg, with the one notable >> difference that we don't set the various legacy PCI attributes for IDE >> and VGA I/O ranges. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 400 ++++++++++++++++++++ >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 56 +++ >> 2 files changed, 456 insertions(+) >> >> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> new file mode 100644 >> index 000000000000..887ddb01f586 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c >> @@ -0,0 +1,400 @@ >> +/** @file >> + PCI Host Bridge Library instance for pci-ecam-generic DT nodes >> + >> + Copyright (c) 2016, Linaro Ltd. All rights reserved.
>> + >> + This program and the accompanying materials are licensed and made available >> + under the terms and conditions of the BSD License which accompanies this >> + distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT >> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#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(0x0A08), // PCI Express > > Differs from 0x0A03, but this change is valid -- not only because we > have PCI Express here, but also because QEMU's ACPI generator produces > PNP0A08 as _HID (and PNP0A03 only as _CID) for PCI0, in acpi_dsdt_add_pci(). > >> + 0 >> + }, >> + >> + { >> + END_DEVICE_PATH_TYPE, >> + END_ENTIRE_DEVICE_PATH_SUBTYPE, >> + { >> + END_DEVICE_PATH_LENGTH, >> + 0 >> + } >> + } >> +}; >> + >> +GLOBAL_REMOVE_IF_UNREFERENCED >> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = { >> + L"Mem", L"I/O", L"Bus" >> +}; >> + >> +// >> +// We expect the "ranges" property of "pci-host-ecam-generic" to consist of >> +// records like this. >> +// >> +#pragma pack (1) >> +typedef struct { >> + UINT32 Type; >> + UINT64 ChildBase; >> + UINT64 CpuBase; >> + UINT64 Size; >> +} DTB_PCI_HOST_RANGE_RECORD; >> +#pragma pack () >> + >> +#define DTB_PCI_HOST_RANGE_RELOCATABLE BIT31 >> +#define DTB_PCI_HOST_RANGE_PREFETCHABLE BIT30 >> +#define DTB_PCI_HOST_RANGE_ALIASED BIT29 >> +#define DTB_PCI_HOST_RANGE_MMIO32 BIT25 >> +#define DTB_PCI_HOST_RANGE_MMIO64 (BIT25 | BIT24) >> +#define DTB_PCI_HOST_RANGE_IO BIT24 >> +#define DTB_PCI_HOST_RANGE_TYPEMASK (BIT31 | BIT30 | BIT29 | BIT25 | BIT24) >> + >> +STATIC >> +EFI_STATUS >> +ProcessPciHost ( >> + OUT UINT64 *IoBase, >> + OUT UINT64 *IoSize, >> + OUT UINT64 *IoTranslation, >> + OUT UINT64 *MmioBase, >> + OUT UINT64 *MmioSize, >> + OUT UINT64 *MmioTranslation, >> + OUT UINT32 *BusMin, >> + OUT UINT32 *BusMax >> + ) >> +{ >> + FDT_CLIENT_PROTOCOL *FdtClient; >> + INT32 Node; >> + UINT64 ConfigBase, ConfigSize; >> + CONST VOID *Prop; >> + UINT32 Len; >> + UINT32 RecordIdx; >> + EFI_STATUS Status; >> + >> + // >> + // The following output arguments are initialized only in >> + // order to suppress '-Werror=maybe-uninitialized' warnings >> + // *incorrectly* emitted by some gcc versions. >> + // >> + *IoBase = 0; >> + *IoTranslation = 0; >> + *MmioBase = 0; >> + *MmioTranslation = 0; >> + *BusMin = 0; >> + *BusMax = 0; >> + >> + // >> + // *IoSize and *MmioSize are initialized to zero because the logic below >> + // requires it. However, since they are also affected by the issue reported >> + // above, they are initialized early. >> + // >> + *IoSize = 0; >> + *MmioSize = 0; >> + >> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, >> + (VOID **)&FdtClient); >> + ASSERT_EFI_ERROR (Status); >> + >> + Status = FdtClient->FindCompatibleNode (FdtClient, "pci-host-ecam-generic", >> + &Node); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((EFI_D_INFO, >> + "%a: No 'pci-host-ecam-generic' compatible DT node found\n", >> + __FUNCTION__)); >> + return EFI_NOT_FOUND; >> + } >> + >> + DEBUG_CODE ( >> + INT32 Tmp; >> + >> + // >> + // A DT can legally describe multiple PCI host bridges, but we are not >> + // equipped to deal with that. So assert that there is only one. >> + // >> + Status = FdtClient->FindNextCompatibleNode (FdtClient, >> + "pci-host-ecam-generic", Node, &Tmp); >> + ASSERT (Status == EFI_NOT_FOUND); >> + ); >> + >> + Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg", &Prop, &Len); >> + if (EFI_ERROR (Status) || Len != 2 * sizeof (UINT64)) { >> + DEBUG ((EFI_D_ERROR, "%a: 'reg' property not found or invalid\n", >> + __FUNCTION__)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // Fetch the ECAM window. >> + // >> + ConfigBase = SwapBytes64 (((CONST UINT64 *)Prop)[0]); >> + ConfigSize = SwapBytes64 (((CONST UINT64 *)Prop)[1]); >> + >> + // >> + // Fetch the bus range (note: inclusive). >> + // >> + Status = FdtClient->GetNodeProperty (FdtClient, Node, "bus-range", &Prop, >> + &Len); >> + if (EFI_ERROR (Status) || Len != 2 * sizeof (UINT32)) { >> + DEBUG ((EFI_D_ERROR, "%a: 'bus-range' not found or invalid\n", >> + __FUNCTION__)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + *BusMin = SwapBytes32 (((CONST UINT32 *)Prop)[0]); >> + *BusMax = SwapBytes32 (((CONST UINT32 *)Prop)[1]); >> + >> + // >> + // Sanity check: the config space must accommodate all 4K register bytes of >> + // all 8 functions of all 32 devices of all buses. >> + // >> + if (*BusMax < *BusMin || *BusMax - *BusMin == MAX_UINT32 || >> + DivU64x32 (ConfigSize, SIZE_4KB * 8 * 32) < *BusMax - *BusMin + 1) { >> + DEBUG ((EFI_D_ERROR, "%a: invalid 'bus-range' and/or 'reg'\n", >> + __FUNCTION__)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // Iterate over "ranges". >> + // >> + Status = FdtClient->GetNodeProperty (FdtClient, Node, "ranges", &Prop, &Len); >> + if (EFI_ERROR (Status) || Len == 0 || >> + Len % sizeof (DTB_PCI_HOST_RANGE_RECORD) != 0) { >> + DEBUG ((EFI_D_ERROR, "%a: 'ranges' not found or invalid\n", __FUNCTION__)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + for (RecordIdx = 0; RecordIdx < Len / sizeof (DTB_PCI_HOST_RANGE_RECORD); >> + ++RecordIdx) { >> + CONST DTB_PCI_HOST_RANGE_RECORD *Record; >> + >> + Record = (CONST DTB_PCI_HOST_RANGE_RECORD *)Prop + RecordIdx; >> + switch (SwapBytes32 (Record->Type) & DTB_PCI_HOST_RANGE_TYPEMASK) { >> + case DTB_PCI_HOST_RANGE_IO: >> + *IoBase = SwapBytes64 (Record->ChildBase); >> + *IoSize = SwapBytes64 (Record->Size); >> + *IoTranslation = SwapBytes64 (Record->CpuBase) - *IoBase; >> + break; >> + >> + case DTB_PCI_HOST_RANGE_MMIO32: >> + *MmioBase = SwapBytes64 (Record->ChildBase); >> + *MmioSize = SwapBytes64 (Record->Size); >> + *MmioTranslation = SwapBytes64 (Record->CpuBase) - *MmioBase; >> + >> + if (*MmioBase > MAX_UINT32 || *MmioSize > MAX_UINT32 || >> + *MmioBase + *MmioSize > SIZE_4GB) { >> + DEBUG ((EFI_D_ERROR, "%a: MMIO32 space invalid\n", __FUNCTION__)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + if (*MmioTranslation != 0) { >> + DEBUG ((EFI_D_ERROR, "%a: unsupported nonzero MMIO32 translation " >> + "0x%Lx\n", __FUNCTION__, *MmioTranslation)); >> + return EFI_UNSUPPORTED; >> + } >> + >> + break; >> + } >> + } >> + if (*IoSize == 0 || *MmioSize == 0) { >> + DEBUG ((EFI_D_ERROR, "%a: %a space empty\n", __FUNCTION__, >> + (*IoSize == 0) ? "IO" : "MMIO32")); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // The dynamic PCD PcdPciExpressBaseAddress should have already been set, >> + // and should match the value we found in the DT node. >> + // >> + ASSERT (PcdGet64 (PcdPciExpressBaseAddress) == ConfigBase); >> + >> + DEBUG ((EFI_D_INFO, "%a: Config[0x%Lx+0x%Lx) Bus[0x%x..0x%x] " >> + "Io[0x%Lx+0x%Lx)@0x%Lx Mem[0x%Lx+0x%Lx)@0x%Lx\n", __FUNCTION__, ConfigBase, >> + ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, *IoTranslation, *MmioBase, >> + *MmioSize, *MmioTranslation)); >> + return EFI_SUCCESS; >> +} > > Okay, the above seems to be preserved identically from > "ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c". > Yes. That is why I decided to remove the linux,pci-probe-only hack first. >> +/** >> + Return all the root bridge instances in an array. >> + >> + @param Count Return the count of root bridge instances. >> + >> + @return All the root bridge instances in an array. >> + The array should be passed into PciHostBridgeFreeRootBridges() >> + when it's not used. >> +**/ >> +PCI_ROOT_BRIDGE * >> +EFIAPI >> +PciHostBridgeGetRootBridges ( >> + UINTN *Count >> + ) >> +{ >> + PCI_ROOT_BRIDGE *RootBridge; >> + UINT64 IoBase, IoSize, IoTranslation; >> + UINT64 Mmio32Base, Mmio32Size, Mmio32Translation; >> + UINT32 BusMin, BusMax; >> + EFI_STATUS Status; >> + >> + if (PcdGet64 (PcdPciExpressBaseAddress) == 0) { >> + DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__)); >> + >> + *Count = 0; >> + return NULL; >> + } >> + >> + Status = ProcessPciHost (&IoBase, &IoSize, &IoTranslation, &Mmio32Base, >> + &Mmio32Size, &Mmio32Translation, &BusMin, &BusMax); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((EFI_D_INFO, > > I propose to turn this into EFI_D_ERROR. At this point > FdtPciPcdProducerLib has located "pci-host-ecam-generic" and set > PcdPciExpressBaseAddress to a nonzero value, so any failure to process > the FDT node correctly should be reported on the ERROR level. > Agreed. >> + "%a: failed to discover PCI host bridge: %s not present\n", >> + __FUNCTION__, Status)); > > The "%s" format spec takes a CHAR16* argument, but Status has type > EFI_STATUS. > > I would replace > > %s not present > > with > > %r > > in the format string. > Ah yes, I fudged that. Will fix. >> + *Count = 0; >> + return NULL; >> + } >> + >> + PcdSet64 (PcdPciIoTranslation, IoTranslation); > > I agree this is necessary, but it's not in the right place, plus as-is, > it is not sufficient. > > In I wrote > -- and I'm slightly reformatting it here, because the formatting got > lost in the GitHub -> BZ migration --: > > The main customization in ArmVirtPkg/PciHostBridgeDxe is that it > emulates IO port accesses with an MMIO range, whose base address is > parsed from the DTB. > > The core PciHostBridgeDxe driver delegates the IO port access > implementation to EFI_CPU_IO2_PROTOCOL. @ardbiesheuvel recently > implemented ArmPkg/Drivers/ArmPciCpuIo2Dxe (in commit 3dece14) > which provides this protocol, backed by the same kind of > translation as described above. The base address comes from > gArmTokenSpaceGuid.PcdPciIoTranslation. > > Therefore: > > (1) We should extend our ArmVirtPkg/Library/FdtPciPcdProducerLib > plugin library to locate the IO translation offset in the DTB, > and store it in the above PCD. > > (2) We should include ArmPciCpuIo2Dxe in the ArmVirtQemu builds, > plugging the above library into it. > > (3) We should implement a PciHostBridgeLib instance, and plug it > into the core PciHostBridgeDxe driver. > > We should create one PCI_ROOT_BRIDGE object, populating it with > the FTD Client code we are currently using in > ArmVirtPkg/PciHostBridgeDxe. > > (4) Extra benefit: the core PciHostBridgeDxe driver supports 64-bit > MMIO, we just have to parse those values from the DTB as well. > > Steps (2) through (4) are implemented by this series, but I don't think > the above PcdSet64() call satisfies (1). What guarantees that by the > time ArmPciCpuIo2Dxe looks at it, PcdPciIoTranslation will be set? > > ... Especially because PciHostBridgeDxe.inf has a DEPEX on > gEfiCpuIo2ProtocolGuid, so the PcdSet64() above is bound to run *after* > ArmPciCpuIo2Dxe is dispatched? > > Hmmmm... Are you making the argument that the PCD is set *between* > ArmPciCpuIo2Dxe's dispatch and actual use of the PCD? That is: > > - ArmPciCpuIo2Dxe is dispatched > - PciHostBridgeDxe is dispatched > - we set the PCD in this lib (as part of PciHostBridgeDxe's startup) > - (much later) something makes a PCI IO or MEM access that causes > PciHostBridgeDxe to talk to ArmPciCpuIo2Dxe > - ArmPciCpuIo2Dxe consumes the PCD > > I think this is a valid argument to make -- I've even checked: > ArmPciCpuIo2Dxe indeed performs a PcdGet64() on every CpuIoServiceRead() > / CpuIoServiceWrite(), and none in its entry point --, but it has to be > explained in detail. > > This borders on a hack -- because ArmPciCpuIo2Dxe is not actually ready > for use when its entry point exits with success --, but I think we can > allow it, both ArmPciCpuIo2Dxe and this lib being platform specific, as > long as we document it profusely. > > So please describe this quirk in the commit message, and add a large > comment right above the PcdSet64() call. > > (Also, did you test this series with std VGA, on TCG? That will actually > put the IO translation to use.) > > The alternative to the commit msg addition / code comment would be my > suggestion in (1), that is, to extend FdtPciPcdProducerLib with setting > PcdPciIoTranslation, and to link it into ArmPciCpuIo2Dxe via NULL > library resolution. I guess you don't like that, which is fine, but then > please add the docs. Thanks. > Actually, I prefer your original suggestion, to add it to FdtPciPcdProducerLib. That way, we no longer have to set any PCDs in this code, which is much cleaner imo > > Independent request (just because I referenced the edk2 BZ above): > please add something like this to all of the commit messages: > > Ref: https://tianocore.acgmultimedia.com/show_bug.cgi?id=65 > Sure >> + >> + *Count = 1; >> + RootBridge = AllocateZeroPool (*Count * sizeof *RootBridge); > > Please check for memory allocation failure here, and return in the usual > way if it fails. > > Alternatively, you can do the exact same thing as with the device path: > give this singleton root bridge object static storage duration, and make > PciHostBridgeFreeRootBridges() a no-op. > For count == 1, that makes sense of course, and we currently don't deal with anything else. I am not sure how likely that is to change, though? I think it has been suggested to allow PCI pass through of non-DMA coherent PCI devices using a separate PCI host, but I am not sure if that is ever going to materialize, especially since host systems that have an SMMU wired to their PCI rc (which I suppose is a requirement for PCI pass through in real-world scenarios) should be able to perform coherent DMA >> + >> + RootBridge->Segment = 0; >> + RootBridge->Supports = 0; >> + RootBridge->Attributes = RootBridge->Supports; > > Hmmm, not so sure about this. This change breaks away from what we > currently have in "ArmVirtPkg/PciHostBridgeDxe/PciRootBridgeIo.c", > function RootBridgeConstructor(). > > The attributes are documented in the UEFI spec (v2.6), "13.2 PCI Root > Bridge I/O Protocol". We do support IO emulation, so I think at least > some of those attributes should be preserved. Honestly, given that we > intend to support std VGA on at least TCG, I would keep all the current > attributes, except the IDE ones. Hmmm, I'd also drop > EFI_PCI_ATTRIBUTE_VGA_MEMORY. So, I'd keep: > > 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 > > According to your commit message, these were dropped deliberately. Can > you please elaborate why we shouldn't keep the above four flags, despite > wanting to support stdvga on TCG? For now I think we should preserve > them, and the commit message should be updated. > I simply thought we didn't need any of them, but I am happy to keep all of them, or just the ones above. >> + >> + RootBridge->DmaAbove4G = TRUE; > > OK, I think. > > ... Here you skip setting NoExtendedConfigSpace explicitly to FALSE. I > think that's fine (due to AllocateZeroPool() above, or else due to the > automatic zero-init of the (proposed) static variable), functionally > speaking. OK. > > Same for ResourceAssigned -- defaults to FALSE, good. > >> + >> + RootBridge->AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM | >> + EFI_PCI_HOST_BRIDGE_MEM64_DECODE ; > > Here the EFI_PCI_HOST_BRIDGE_MEM64_DECODE bit belongs to patch #4 ("add > MMIO64 support"), not here, in my opinion. It should depend on whether > the FDT provides a 64-bit MMIO range. > OK, will fix that > (Also, gratuitous whitespace before the semicolon.) > >> + >> + RootBridge->Bus.Base = BusMin; >> + RootBridge->Bus.Limit = BusMax; >> + RootBridge->Io.Base = IoBase; >> + RootBridge->Io.Limit = IoBase + IoSize - 1; >> + RootBridge->Mem.Base = Mmio32Base; >> + RootBridge->Mem.Limit = Mmio32Base + Mmio32Size - 1; >> + RootBridge->MemAbove4G.Base = 0x100000000ULL; >> + RootBridge->MemAbove4G.Limit = 0xFFFFFFFF; > > This is valid for disabling the 64-bit MMIO range (temporarily), but > it's not consistent with how the prefetchable stuff is disabled below > (with MAX_UINT64 and 0). I suspect this is going to be modified in patch > #4 anyway, but I suggest to keep the consistency here. > Actually, I fixed that up but folded the hunk into the wrong patch. >> + >> + // >> + // No separate ranges for prefetchable and non-prefetchable BARs >> + // >> + RootBridge->PMem.Base = MAX_UINT64; >> + RootBridge->PMem.Limit = 0; >> + RootBridge->PMemAbove4G.Base = MAX_UINT64; >> + RootBridge->PMemAbove4G.Limit = 0; >> + >> + ASSERT (FixedPcdGet64 (PcdPciMmio32Translation) == 0); >> + ASSERT (FixedPcdGet64 (PcdPciMmio64Translation) == 0); > > OK. > >> + >> + RootBridge->NoExtendedConfigSpace = FALSE; > > Ah! So here it is. How about moving it up to where the structure > definition would suggest it? Not too important, of course. > >> + >> + RootBridge->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath; >> + >> + return RootBridge; >> +} > > Looks good. > >> + >> +/** >> + Free the root bridge instances array returned from PciHostBridgeGetRootBridges(). > > This line is too long. Doesn't justify a repost on its own, but if you > repost for other reasons, it would be nice to wrap this. > >> + >> + @param Bridges The root bridge instances array. >> + @param Count The count of the array. >> +**/ >> +VOID >> +EFIAPI >> +PciHostBridgeFreeRootBridges ( >> + PCI_ROOT_BRIDGE *Bridges, >> + UINTN Count >> + ) >> +{ >> + FreePool (Bridges); >> +} > > Good if we stick with AllocateZeroPool() -- augmented with error > checking --; otherwise, if you opt for the static object, this should > become an empty function. > >> + >> +/** >> + Inform the platform that the resource conflict happens. >> + >> + @param HostBridgeHandle Handle of the Host Bridge. >> + @param Configuration Pointer to PCI I/O and PCI memory resource >> + descriptors. The Configuration contains the resources >> + for all the root bridges. The resource for each root >> + bridge is terminated with END descriptor and an >> + additional END is appended indicating the end of the >> + entire resources. The resource descriptor field >> + values follow the description in >> + EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL >> + .SubmitResources(). >> +**/ >> +VOID >> +EFIAPI >> +PciHostBridgeResourceConflict ( >> + EFI_HANDLE HostBridgeHandle, >> + VOID *Configuration >> + ) >> +{ >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; >> + UINTN RootBridgeIndex; >> + DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n")); >> + >> + RootBridgeIndex = 0; >> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration; >> + while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { >> + DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++)); >> + for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) { >> + ASSERT (Descriptor->ResType < >> + (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) / >> + sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0]) >> + ) >> + ); >> + DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n", >> + mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType], >> + Descriptor->AddrLen, Descriptor->AddrRangeMax >> + )); >> + if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { >> + DEBUG ((EFI_D_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n", >> + Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag, >> + ((Descriptor->SpecificFlag & >> + EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE >> + ) != 0) ? L" (Prefetchable)" : L"" >> + )); >> + } >> + } >> + // >> + // Skip the END descriptor for root bridge >> + // >> + ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR); >> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)( >> + (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1 >> + ); >> + } >> +} > > Okay, this function is directly copied from > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c". > >> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf >> new file mode 100644 >> index 000000000000..16beef0c2425 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf >> @@ -0,0 +1,56 @@ >> +## @file >> +# PCI Host Bridge Library instance for pci-ecam-generic DT nodes >> +# >> +# Copyright (c) 2016, Linaro Ltd. All rights reserved.
>> +# >> +# This program and the accompanying materials are licensed and made available >> +# under the terms and conditions of the BSD License which accompanies this >> +# distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> +# IMPLIED. >> +# >> +# >> +## >> + >> +[Defines] >> + INF_VERSION = 0x00010005 >> + BASE_NAME = AmdStyxPciHostBridgeLib > > The BASE_NAME is wrong. > Oops >> + FILE_GUID = 05E7AB83-EF8D-482D-80F8-905B73377A15 >> + MODULE_TYPE = BASE > > I think this should be DXE_DRIVER, for consistency. The parameter list > of the entry point function is irrelevant here, but in a BASE type > library, we are supposed to use RETURN_STATUS, not EFI_STATUS (although > those types and their values are identical). The C file added above uses > EFI_STATUS in a bunch of places, so I'd make the module type DXE_DRIVER, > for consistency. > Sure >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = PciHostBridgeLib >> + >> +# >> +# The following information is for reference only and not required by the build >> +# tools. >> +# >> +# VALID_ARCHITECTURES = AARCH64 > > Not to be used in the ARM build of ArmVirtQemu? > Actually, it should but I haven't tried. >> +# >> + >> +[Sources] >> + FdtPciHostBridgeLib.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + ArmPkg/ArmPkg.dec >> + ArmVirtPkg/ArmVirtPkg.dec >> + >> +[LibraryClasses] >> + DebugLib >> + DevicePathLib >> + MemoryAllocationLib >> + PciPcdProducerLib >> + >> +[FixedPcd] >> + gArmTokenSpaceGuid.PcdPciMmio32Translation >> + gArmTokenSpaceGuid.PcdPciMmio64Translation >> + >> +[Pcd] >> + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress >> + gArmTokenSpaceGuid.PcdPciIoTranslation >> + >> +[Depex] >> + gFdtClientProtocolGuid >> > > Thanks! > Laszlo