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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 27 Nov 2018 at 18:40, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.
>

Yeah that makes sense

> > +
> >  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
> )
>

Indeed. Well spotted.

> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  return Status;
> >  }
> >
> >  STATIC PCI_ROOT_BRIDGE mRootBridge;
> >
>
> With (2) fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks.