diff mbox series

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

Message ID 20181127145418.11992-2-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series ArmVirtPkg: remove high peripheral space mapping | expand

Commit Message

Ard Biesheuvel Nov. 27, 2018, 2:54 p.m. UTC 
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(-)

-- 
2.19.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Philippe Mathieu-Daudé Nov. 27, 2018, 3:28 p.m. UTC | #1 
On 27/11/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;
> +}
> +
>  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);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
>  }
>  
>  STATIC PCI_ROOT_BRIDGE mRootBridge;
> 

LGTM:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Laszlo Ersek Nov. 27, 2018, 5:40 p.m. UTC | #2 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 27, 2018, 5:47 p.m. UTC | #3 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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;
+}
+
 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);
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
 }
 
 STATIC PCI_ROOT_BRIDGE mRootBridge;