| Message ID | 1471847752-26574-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > 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.<BR> > + > + 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 <PiDxe.h> > +#include <Library/PciHostBridgeLib.h> > +#include <Library/DebugLib.h> > +#include <Library/DevicePathLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/PcdLib.h> > +#include <Library/UefiBootServicesTableLib.h> > + > +#include <Protocol/FdtClient.h> > +#include <Protocol/PciRootBridgeIo.h> > +#include <Protocol/PciHostBridgeResourceAllocation.h> > + > +#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". > +/** > + 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. > + "%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. > + *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 <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> 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. 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 > + > + *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. > + > + 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. > + > + 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. (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. > + > + // > + // 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.<BR> > +# > +# 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. > + 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. > + 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? > +# > + > +[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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org> >> --- >> 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.<BR> >> + >> + 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 <PiDxe.h> >> +#include <Library/PciHostBridgeLib.h> >> +#include <Library/DebugLib.h> >> +#include <Library/DevicePathLib.h> >> +#include <Library/MemoryAllocationLib.h> >> +#include <Library/PcdLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> + >> +#include <Protocol/FdtClient.h> >> +#include <Protocol/PciRootBridgeIo.h> >> +#include <Protocol/PciHostBridgeResourceAllocation.h> >> + >> +#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 <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> 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.<BR> >> +# >> +# 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 08/24/16 17:19, Ard Biesheuvel wrote: > On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote: >> On 08/22/16 02:35, Ard Biesheuvel wrote: >>> + PcdSet64 (PcdPciIoTranslation, IoTranslation); >> >> I agree this is necessary, but it's not in the right place, plus as-is, >> it is not sufficient. >> >> In <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> 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 Sounds good, thanks! In this case, the IoTranslation variable should become local to ProcessPciHost() -- and ProcessPciHost() only -- in this patch, plus I think we should also add ASSERT (PcdGet64 (PcdPciIoTranslation) == IoTranslation); to ProcessPciHost() here, under the DTB_PCI_HOST_RANGE_IO branch. Furthermore, the zero-assignment of the now-local IoTranslation variable should probably be moved out from under the '-Werror=maybe-uninitialized' comment, because it won't be an output parameter any longer. What do you think? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 31 August 2016 at 14:51, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/24/16 17:19, Ard Biesheuvel wrote: >> On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 08/22/16 02:35, Ard Biesheuvel wrote: > >>>> + PcdSet64 (PcdPciIoTranslation, IoTranslation); >>> >>> I agree this is necessary, but it's not in the right place, plus as-is, >>> it is not sufficient. >>> >>> In <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> 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 > > Sounds good, thanks! > > In this case, the IoTranslation variable should become local to > ProcessPciHost() -- and ProcessPciHost() only -- in this patch, plus I > think we should also add > > ASSERT (PcdGet64 (PcdPciIoTranslation) == IoTranslation); > > to ProcessPciHost() here, under the DTB_PCI_HOST_RANGE_IO branch. > > Furthermore, the zero-assignment of the now-local IoTranslation variable > should probably be moved out from under the > '-Werror=maybe-uninitialized' comment, because it won't be an output > parameter any longer. What do you think? > Yes, that was my idea as well. The FdtPciPcdProducerLib will need some work, but it is ultimately a much cleaner approach. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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.<BR> + + 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 <PiDxe.h> +#include <Library/PciHostBridgeLib.h> +#include <Library/DebugLib.h> +#include <Library/DevicePathLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/PcdLib.h> +#include <Library/UefiBootServicesTableLib.h> + +#include <Protocol/FdtClient.h> +#include <Protocol/PciRootBridgeIo.h> +#include <Protocol/PciHostBridgeResourceAllocation.h> + +#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 + 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; +} + + +/** + 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, + "%a: failed to discover PCI host bridge: %s not present\n", + __FUNCTION__, Status)); + *Count = 0; + return NULL; + } + + PcdSet64 (PcdPciIoTranslation, IoTranslation); + + *Count = 1; + RootBridge = AllocateZeroPool (*Count * sizeof *RootBridge); + + RootBridge->Segment = 0; + RootBridge->Supports = 0; + RootBridge->Attributes = RootBridge->Supports; + + RootBridge->DmaAbove4G = TRUE; + + RootBridge->AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM | + EFI_PCI_HOST_BRIDGE_MEM64_DECODE ; + + 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; + + // + // 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); + + RootBridge->NoExtendedConfigSpace = FALSE; + + RootBridge->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath; + + return RootBridge; +} + +/** + Free the root bridge instances array returned from PciHostBridgeGetRootBridges(). + + @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); +} + +/** + 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 + ); + } +} 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.<BR> +# +# 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 + FILE_GUID = 05E7AB83-EF8D-482D-80F8-905B73377A15 + MODULE_TYPE = BASE + VERSION_STRING = 1.0 + LIBRARY_CLASS = PciHostBridgeLib + +# +# The following information is for reference only and not required by the build +# tools. +# +# VALID_ARCHITECTURES = AARCH64 +# + +[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
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 <ard.biesheuvel@linaro.org> --- ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 400 ++++++++++++++++++++ ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 56 +++ 2 files changed, 456 insertions(+) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel