On 11/27/18 15:54, Ard Biesheuvel wrote:
> Up until now, we have been getting away with not declaring the ECAM
> and translated I/O spaces at all in the GCD memory map, simply because
> we map the entire address space with device attributes in the early PEI
> code, and so these regions will be mapped wherever they end up.
>
> Now that we are about to make changes to how ArmVirtQemu reasons
> about the size of the address space, it would be better to get rid
> of this mapping of the entire address space, since it can get
> arbitrarily large without real benefit.
>
> So start by mapping the ECAM and translated I/O spaces explicitly,
> instead of relying on the early PEI mapping.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 1 +
> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 46
> +++++++++++++++++++-
> 2 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> index 0995f4b7a156..4011336a353b 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
> @@ -42,6 +42,7 @@ [Packages]
> [LibraryClasses]
> DebugLib
> DevicePathLib
> + DxeServicesTableLib
> MemoryAllocationLib
> PciPcdProducerLib
>
> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> index 5b9c887db35d..ba177353122e 100644
> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
> @@ -17,6 +17,7 @@
> #include <Library/PciHostBridgeLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> +#include <Library/DxeServicesTableLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> @@ -82,6 +83,33 @@ typedef struct {
> #define DTB_PCI_HOST_RANGE_IO BIT24
> #define DTB_PCI_HOST_RANGE_TYPEMASK (BIT31 | BIT30 | BIT29 | BIT25 |
> BIT24)
>
> +STATIC
> +EFI_STATUS
> +MapGcdMmioSpace (
> + IN UINT64 Base,
> + IN UINT64 Size
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, Base, Size,
> + EFI_MEMORY_UC);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN,
> + "%a: failed to add GCD memory space for region [0x%Lx+0x%Lx)\n",
> + __FUNCTION__, Base, Size));
> + return Status;
> + }
> +
> + Status = gDS->SetMemorySpaceAttributes (Base, Size, EFI_MEMORY_UC);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN,
> + "%a: failed to set memory space attributes for region [0x%Lx+0x%Lx)\n",
> + __FUNCTION__, Base, Size));
> + }
> + return Status;
> +}
(1) Given that these failures will quite directly trigger assertions
(which print messages even in such builds that filter out everything
except DEBUG_ERROR), I wonder if we should use DEBUG_ERROR here.
Anyway, just an idea, up to you.
> +
> STATIC
> EFI_STATUS
> ProcessPciHost (
> @@ -266,7 +294,23 @@ ProcessPciHost (
> "Io[0x%Lx+0x%Lx)@0x%Lx Mem32[0x%Lx+0x%Lx)@0x0 Mem64[0x%Lx+0x%Lx)@0x0\n",
> __FUNCTION__, ConfigBase, ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize,
> IoTranslation, *Mmio32Base, *Mmio32Size, *Mmio64Base, *Mmio64Size));
> - return EFI_SUCCESS;
> +
> + // Map the ECAM space in the GCD memory map
> + Status = MapGcdMmioSpace (ConfigBase, ConfigSize);
> + ASSERT_EFI_ERROR (Status);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Map the MMIO window that provides I/O access - the PCI host bridge code
> + // is not aware of this translation and so it will only map the I/O view
> + // in the GCD I/O map.
> + //
> + Status = MapGcdMmioSpace (IoTranslation, *IoSize);
(2) I think using IoTranslation as base address is incorrect here. I
reviewed the explanation in "ArmPkg/ArmPkg.dec", and also the rest of
the code in this function (which matches the DEC's specification). Thus,
I think you need, for the CPU view, (*IoBase + IoTranslation), as first
argument.
(I do agree that the DEBUG message right at the end of the function
could be misleading; it prints
Io[0x%Lx+0x%Lx)@0x%Lx
from
*IoBase, *IoSize, IoTranslation
)
> + ASSERT_EFI_ERROR (Status);
> +
> + return Status;
> }
>
> STATIC PCI_ROOT_BRIDGE mRootBridge;
>
With (2) fixed:
Reviewed-by: Laszlo Ersek <[email protected]>
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
