Re: [PATCH v2 1/2] ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory map

[Laszlo Ersek <lersek@redhat.com>]

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 <ard.biesheuvel@linaro.org>
> ---
>  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 <lersek@redhat.com>

Thanks,
Laszlo