On Tue, 27 Nov 2018 at 18:40, Laszlo Ersek <[email protected]> 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 <[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.
>
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 <[email protected]>
>
Thanks.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
