[PATCH v2 0/2] ArmVirtPkg: remove high peripheral space mapping

[PATCH v2 0/2] ArmVirtPkg: remove high peripheral space mapping

[Ard Biesheuvel]

Stop mapping the entire address space in the early PEI code. This
wastes temporary RAM on page tables, and encourages sloppy coding
when it comes to populating the GCD memory map.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Philippe Mathieu-Daude <philmd@redhat.com>

Ard Biesheuvel (2):
  ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory
    map
  ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

 .../FdtPciHostBridgeLib.inf                   |  1 +
 .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 46 ++++++++++++++++++-
 .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   | 17 ++-----
 3 files changed, 51 insertions(+), 13 deletions(-)

-- 
2.19.1

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

[Ard Biesheuvel]

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;
-- 
2.19.1

[PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Ard Biesheuvel]

Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
entire virtual address space is mapped with EFI_MEMORY_UC attributes,
regardless of whether any devices actually reside there.

Now that we are relaxing the address space limit to more than 40 bits,
mapping all that address space actually takes up more space in page
tables than we have so far made available as temporary RAM. So let's
get rid of the mapping rather than increasing the available RAM, given
that the mapping is not particularly useful anyway.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 815ca145b644..70863abb2e7b 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
   VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
-  // Peripheral space after DRAM
-  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
-  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
-                                       VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
-
   // Remap the FD region as normal executable memory
-  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
-  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
-  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
-  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
+  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
+  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
 
   // End of Table
-  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
 
   *VirtualMemoryMap = VirtualMemoryTable;
 }
-- 
2.19.1

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

[Philippe Mathieu-Daudé]

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>

Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Laszlo Ersek]

On 11/27/18 15:54, Ard Biesheuvel wrote:
> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
> entire virtual address space is mapped with EFI_MEMORY_UC attributes,
> regardless of whether any devices actually reside there.
> 
> Now that we are relaxing the address space limit to more than 40 bits,
> mapping all that address space actually takes up more space in page
> tables than we have so far made available as temporary RAM. So let's
> get rid of the mapping rather than increasing the available RAM, given
> that the mapping is not particularly useful anyway.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> index 815ca145b644..70863abb2e7b 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>  
> -  // Peripheral space after DRAM
> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
> -                                       VirtualMemoryTable[2].PhysicalBase;
> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> -
>    // Remap the FD region as normal executable memory
> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>    // End of Table
> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>  
>    *VirtualMemoryMap = VirtualMemoryTable;
>  }
> 

(1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

(2) Regarding the patch itself. Currently we have:

- VirtualMemoryTable[0]: "System DRAM"
- VirtualMemoryTable[1]: "Peripheral space before DRAM"
- VirtualMemoryTable[2]: "Peripheral space after DRAM"
- VirtualMemoryTable[3]: "Remap the FD region as normal executable
                          memory"

Let's see what is affected, from the physical map in QEMU's "hw/arm/virt.c", if we evict VirtualMemoryTable[2]:

    /* Additional 64 MB redist region (can contain up to 512 redistributors) */
    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x4000000 },
    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
    /* Second PCIe window, 512GB wide at the 512GB boundary */
    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },

I have no idea about VIRT_GIC_REDIST2, but, given that in ArmVirtQemu we do uniprocessor only, it doesn't seem worrisome.

VIRT_PCIE_ECAM_HIGH should be handled by patch #1. (VIRT_PCIE_ECAM_HIGH *replaces* [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, if memory serves.)

VIRT_PCIE_MMIO_HIGH is *in addition* to [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, but we need not do anything about that specifically, because we advertize it to PciHostBridgeDxe via our FdtPciHostBridgeLib instance, and PciHostBridgeDxe handles the GCD aspects for the range automatically.

So, together with patch #1, I think this is safe. If we catch a data abort anyway, we'll have to clean up the GCD handling in other drivers.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
LAszlo

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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.

Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Ard Biesheuvel]

On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/27/18 15:54, Ard Biesheuvel wrote:
> > Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
> > entire virtual address space is mapped with EFI_MEMORY_UC attributes,
> > regardless of whether any devices actually reside there.
> >
> > Now that we are relaxing the address space limit to more than 40 bits,
> > mapping all that address space actually takes up more space in page
> > tables than we have so far made available as temporary RAM. So let's
> > get rid of the mapping rather than increasing the available RAM, given
> > that the mapping is not particularly useful anyway.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > index 815ca145b644..70863abb2e7b 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
> >    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
> >    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> > -  // Peripheral space after DRAM
> > -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> > -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> > -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
> > -                                       VirtualMemoryTable[2].PhysicalBase;
> > -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > -
> >    // Remap the FD region as normal executable memory
> > -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> > -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
> > -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
> > -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> > +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> > +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
> > +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >
> >    // End of Table
> > -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> > +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> >
> >    *VirtualMemoryMap = VirtualMemoryTable;
> >  }
> >
>
> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?
>

Not quite. It complements it, in the sense that is should fix the
issue reported by Eric when mapping the entire address 48-bit address
space.

> (2) Regarding the patch itself. Currently we have:
>
> - VirtualMemoryTable[0]: "System DRAM"
> - VirtualMemoryTable[1]: "Peripheral space before DRAM"
> - VirtualMemoryTable[2]: "Peripheral space after DRAM"
> - VirtualMemoryTable[3]: "Remap the FD region as normal executable
>                           memory"
>
> Let's see what is affected, from the physical map in QEMU's "hw/arm/virt.c", if we evict VirtualMemoryTable[2]:
>
>     /* Additional 64 MB redist region (can contain up to 512 redistributors) */
>     [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x4000000 },
>     [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
>     /* Second PCIe window, 512GB wide at the 512GB boundary */
>     [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>
> I have no idea about VIRT_GIC_REDIST2, but, given that in ArmVirtQemu we do uniprocessor only, it doesn't seem worrisome.
>

The GICv3 architecture permits redistributors (one for each CPU) to be
non-contiguous in physical memory. Some multi-socket systems make use
of this.

In ArmVirtQemu (or actually, in EDK2 in general) we assume that the
boot CPU's redistributor is in the primary redistributor region, so
this region can indeed be disregarded.

> VIRT_PCIE_ECAM_HIGH should be handled by patch #1. (VIRT_PCIE_ECAM_HIGH *replaces* [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, if memory serves.)
>
> VIRT_PCIE_MMIO_HIGH is *in addition* to [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, but we need not do anything about that specifically, because we advertize it to PciHostBridgeDxe via our FdtPciHostBridgeLib instance, and PciHostBridgeDxe handles the GCD aspects for the range automatically.
>
> So, together with patch #1, I think this is safe. If we catch a data abort anyway, we'll have to clean up the GCD handling in other drivers.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks

Eric, mind applying this to double check that it fixes your issue?
(and doesn't break anything else?)

Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Laszlo Ersek]

On 11/27/18 18:52, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 11/27/18 15:54, Ard Biesheuvel wrote:
>>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
>>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,
>>> regardless of whether any devices actually reside there.
>>>
>>> Now that we are relaxing the address space limit to more than 40 bits,
>>> mapping all that address space actually takes up more space in page
>>> tables than we have so far made available as temporary RAM. So let's
>>> get rid of the mapping rather than increasing the available RAM, given
>>> that the mapping is not particularly useful anyway.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
>>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>> index 815ca145b644..70863abb2e7b 100644
>>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
>>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
>>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>
>>> -  // Peripheral space after DRAM
>>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
>>> -                                       VirtualMemoryTable[2].PhysicalBase;
>>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> -
>>>    // Remap the FD region as normal executable memory
>>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
>>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
>>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>>
>>>    // End of Table
>>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>>
>>>    *VirtualMemoryMap = VirtualMemoryTable;
>>>  }
>>>
>>
>> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?
>>
> 
> Not quite. It complements it, in the sense that is should fix the
> issue reported by Eric when mapping the entire address 48-bit address
> space.

Oh, you meant this one *on top* of that? In particular, on top of:

[edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore
                        PcdPrePiCpuMemorySize

That wasn't clear to me, sorry.

If this one comes on top of the v2 13-part series, do you ultimately
need v2 11/13 as a separate patch -- in that form anyway? It seems that
you could squash this patch into v2 11/13, and eliminate the dependency
on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral
space after DRAM.

Thanks,
Laszlo

Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Ard Biesheuvel]

On Tue, 27 Nov 2018 at 21:25, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/27/18 18:52, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 11/27/18 15:54, Ard Biesheuvel wrote:
> >>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
> >>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,
> >>> regardless of whether any devices actually reside there.
> >>>
> >>> Now that we are relaxing the address space limit to more than 40 bits,
> >>> mapping all that address space actually takes up more space in page
> >>> tables than we have so far made available as temporary RAM. So let's
> >>> get rid of the mapping rather than increasing the available RAM, given
> >>> that the mapping is not particularly useful anyway.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
> >>>  1 file changed, 5 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> >>> index 815ca145b644..70863abb2e7b 100644
> >>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> >>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> >>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
> >>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
> >>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>>
> >>> -  // Peripheral space after DRAM
> >>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> >>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> >>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
> >>> -                                       VirtualMemoryTable[2].PhysicalBase;
> >>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>> -
> >>>    // Remap the FD region as normal executable memory
> >>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> >>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
> >>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
> >>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
> >>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> >>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
> >>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >>>
> >>>    // End of Table
> >>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> >>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> >>>
> >>>    *VirtualMemoryMap = VirtualMemoryTable;
> >>>  }
> >>>
> >>
> >> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?
> >>
> >
> > Not quite. It complements it, in the sense that is should fix the
> > issue reported by Eric when mapping the entire address 48-bit address
> > space.
>
> Oh, you meant this one *on top* of that? In particular, on top of:
>
> [edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore
>                         PcdPrePiCpuMemorySize
>
> That wasn't clear to me, sorry.
>

No, the other way around actually :-)

Apologies, I managed to confuse myself a bit as well, so I understand
this may be slightly difficult to follow.

> If this one comes on top of the v2 13-part series, do you ultimately
> need v2 11/13 as a separate patch -- in that form anyway? It seems that
> you could squash this patch into v2 11/13, and eliminate the dependency
> on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral
> space after DRAM.
>

Indeed. So after applying these two patches, I will need to respin
that series once more, and now that I think of it, it might make sense
to simplify those changes signficantly, given that only the Xen code
needs to access the CPU's capability registers in the platform MMU
setup code.

Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Laszlo Ersek]

On 11/27/18 22:18, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 21:25, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 11/27/18 18:52, Ard Biesheuvel wrote:
>>> On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>
>>>> On 11/27/18 15:54, Ard Biesheuvel wrote:
>>>>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
>>>>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,
>>>>> regardless of whether any devices actually reside there.
>>>>>
>>>>> Now that we are relaxing the address space limit to more than 40 bits,
>>>>> mapping all that address space actually takes up more space in page
>>>>> tables than we have so far made available as temporary RAM. So let's
>>>>> get rid of the mapping rather than increasing the available RAM, given
>>>>> that the mapping is not particularly useful anyway.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
>>>>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> index 815ca145b644..70863abb2e7b 100644
>>>>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
>>>>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
>>>>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>>>
>>>>> -  // Peripheral space after DRAM
>>>>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>>>>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>>>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
>>>>> -                                       VirtualMemoryTable[2].PhysicalBase;
>>>>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>>> -
>>>>>    // Remap the FD region as normal executable memory
>>>>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>>>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>>>>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
>>>>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>>>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>>>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>>>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
>>>>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>>>>
>>>>>    // End of Table
>>>>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>>>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>>>>
>>>>>    *VirtualMemoryMap = VirtualMemoryTable;
>>>>>  }
>>>>>
>>>>
>>>> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?
>>>>
>>>
>>> Not quite. It complements it, in the sense that is should fix the
>>> issue reported by Eric when mapping the entire address 48-bit address
>>> space.
>>
>> Oh, you meant this one *on top* of that? In particular, on top of:
>>
>> [edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore
>>                         PcdPrePiCpuMemorySize
>>
>> That wasn't clear to me, sorry.
>>
> 
> No, the other way around actually :-)

How so? Patch v2 11/13 removes:

> -  VirtualMemoryTable[2].Length       = TopOfMemory -

and adds:

> +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

and in the current patch, you remove

> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

So the current patch wouldn't apply before v2 11/13.

Anyway, this is not so important :)

> Apologies, I managed to confuse myself a bit as well, so I understand
> this may be slightly difficult to follow.

Yeah :)

>> If this one comes on top of the v2 13-part series, do you ultimately
>> need v2 11/13 as a separate patch -- in that form anyway? It seems that
>> you could squash this patch into v2 11/13, and eliminate the dependency
>> on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral
>> space after DRAM.
>>
> 
> Indeed. So after applying these two patches, I will need to respin
> that series once more, and now that I think of it, it might make sense
> to simplify those changes signficantly, given that only the Xen code
> needs to access the CPU's capability registers in the platform MMU
> setup code.

Thank you for explaining. I'll wait for the one and only v3 then.

Thanks!
Laszlo