[PATCH v2 0/2] Add Local APIC memory mapped space in GCD

[PATCH v2 0/2] Add Local APIC memory mapped space in GCD

[Jeff Fan]

Local APIC memory mapped space should be added into GCD and be allocated.
Otherwise, UEFI firmware cannot get correct memory map for it. For example,
SMM profile feature needs to get the completed MMIO map to protect them.

v2:
  Consume AddMemoryMappedIoSpace() to handle the case that Local APIC
  memory space has already been added before per Laszlo's comments at:
  https://www.mail-archive.com/edk2-devel@lists.01.org/msg22561.html
  Tested on OvmfPkg.

https://bugzilla.tianocore.org/show_bug.cgi?id=390

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>

Jeff Fan (2):
  UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge
  UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in GCD

 UefiCpuPkg/CpuDxe/CpuDxe.c | 183 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

-- 
2.9.3.windows.2

[PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

[Jeff Fan]

Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from
MdeModulePkg\Bus\Pci\PciHostBridgeDxe\PciHostBridge.c.

https://bugzilla.tianocore.org/show_bug.cgi?id=390

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuDxe.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 9fb6d76..925c50f 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -887,6 +887,154 @@ IdleLoopEventCallback (
   CpuSleep ();
 }
 
+/**
+  Ensure the compatibility of a memory space descriptor with the MMIO aperture.
+
+  The memory space descriptor can come from the GCD memory space map, or it can
+  represent a gap between two neighboring memory space descriptors. In the
+  latter case, the GcdMemoryType field is expected to be
+  EfiGcdMemoryTypeNonExistent.
+
+  If the memory space descriptor already has type
+  EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset of the
+  required capabilities, then no action is taken -- it is by definition
+  compatible with the aperture.
+
+  Otherwise, the intersection of the memory space descriptor is calculated with
+  the aperture. If the intersection is the empty set (no overlap), no action is
+  taken; the memory space descriptor is compatible with the aperture.
+
+  Otherwise, the type of the descriptor is investigated again. If the type is
+  EfiGcdMemoryTypeNonExistent (representing a gap, or a genuine descriptor with
+  such a type), then an attempt is made to add the intersection as MMIO space
+  to the GCD memory space map, with the specified capabilities. This ensures
+  continuity for the aperture, and the descriptor is deemed compatible with the
+  aperture.
+
+  Otherwise, the memory space descriptor is incompatible with the MMIO
+  aperture.
+
+  @param[in] Base         Base address of the aperture.
+  @param[in] Length       Length of the aperture.
+  @param[in] Capabilities Capabilities required by the aperture.
+  @param[in] Descriptor   The descriptor to ensure compatibility with the
+                          aperture for.
+
+  @retval EFI_SUCCESS            The descriptor is compatible. The GCD memory
+                                 space map may have been updated, for
+                                 continuity within the aperture.
+  @retval EFI_INVALID_PARAMETER  The descriptor is incompatible.
+  @return                        Error codes from gDS->AddMemorySpace().
+**/
+EFI_STATUS
+IntersectMemoryDescriptor (
+  IN  UINT64                                Base,
+  IN  UINT64                                Length,
+  IN  UINT64                                Capabilities,
+  IN  CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor
+  )
+{
+  UINT64                                    IntersectionBase;
+  UINT64                                    IntersectionEnd;
+  EFI_STATUS                                Status;
+
+  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo &&
+      (Descriptor->Capabilities & Capabilities) == Capabilities) {
+    return EFI_SUCCESS;
+  }
+
+  IntersectionBase = MAX (Base, Descriptor->BaseAddress);
+  IntersectionEnd = MIN (Base + Length,
+                      Descriptor->BaseAddress + Descriptor->Length);
+  if (IntersectionBase >= IntersectionEnd) {
+    //
+    // The descriptor and the aperture don't overlap.
+    //
+    return EFI_SUCCESS;
+  }
+
+  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+    Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
+                    IntersectionBase, IntersectionEnd - IntersectionBase,
+                    Capabilities);
+
+    DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE,
+      "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__,
+      IntersectionBase, IntersectionEnd, Status));
+    return Status;
+  }
+
+  DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflicts "
+    "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__,
+    Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length,
+    (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities,
+    Base, Base + Length, Capabilities));
+  return EFI_INVALID_PARAMETER;
+}
+
+/**
+  Add MMIO space to GCD.
+  The routine checks the GCD database and only adds those which are
+  not added in the specified range to GCD.
+
+  @param Base         Base address of the MMIO space.
+  @param Length       Length of the MMIO space.
+  @param Capabilities Capabilities of the MMIO space.
+
+  @retval EFI_SUCCES The MMIO space was added successfully.
+**/
+EFI_STATUS
+AddMemoryMappedIoSpace (
+  IN  UINT64                            Base,
+  IN  UINT64                            Length,
+  IN  UINT64                            Capabilities
+  )
+{
+  EFI_STATUS                            Status;
+  UINTN                                 Index;
+  UINTN                                 NumberOfDescriptors;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       *MemorySpaceMap;
+
+  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n",
+      gEfiCallerBaseName, __FUNCTION__, Status));
+    return Status;
+  }
+
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    Status = IntersectMemoryDescriptor (Base, Length, Capabilities,
+               &MemorySpaceMap[Index]);
+    if (EFI_ERROR (Status)) {
+      goto FreeMemorySpaceMap;
+    }
+  }
+
+  DEBUG_CODE (
+    //
+    // Make sure there are adjacent descriptors covering [Base, Base + Length).
+    // It is possible that they have not been merged; merging can be prevented
+    // by allocation and different capabilities.
+    //
+    UINT64                          CheckBase;
+    EFI_STATUS                      CheckStatus;
+    EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor;
+
+    for (CheckBase = Base;
+         CheckBase < Base + Length;
+         CheckBase = Descriptor.BaseAddress + Descriptor.Length) {
+      CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor);
+      ASSERT_EFI_ERROR (CheckStatus);
+      ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo);
+      ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities);
+    }
+    );
+
+FreeMemorySpaceMap:
+  FreePool (MemorySpaceMap);
+
+  return Status;
+}
 
 /**
   Initialize the state information for the CPU Architectural Protocol.
-- 
2.9.3.windows.2

Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

[Laszlo Ersek]

I have two suggestions for this patch:

On 02/21/17 04:08, Jeff Fan wrote:
> Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from
> MdeModulePkg\Bus\Pci\PciHostBridgeDxe\PciHostBridge.c.

(1) One of the two "AddMemoryMappedIoSpace" strings should be replaced
with "IntersectMemoryDescriptor".

This can be done when you commit / push the patch; no need to repost
just because of it.

(2) This is more of a "followup" suggestion: since this is new code, I
recommend that the EFI_D_ macros be replced with DEBUG_ macros.

However, I see value in keeping the first patch as a pristine copy from
the PciHostBridgeDxe driver, so I don't recommend doing the above change
in patch #1. I think it can be patch #3, or a completely separate patch
(later).

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

Thanks!
Laszlo

> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=390
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 9fb6d76..925c50f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -887,6 +887,154 @@ IdleLoopEventCallback (
>    CpuSleep ();
>  }
>  
> +/**
> +  Ensure the compatibility of a memory space descriptor with the MMIO aperture.
> +
> +  The memory space descriptor can come from the GCD memory space map, or it can
> +  represent a gap between two neighboring memory space descriptors. In the
> +  latter case, the GcdMemoryType field is expected to be
> +  EfiGcdMemoryTypeNonExistent.
> +
> +  If the memory space descriptor already has type
> +  EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset of the
> +  required capabilities, then no action is taken -- it is by definition
> +  compatible with the aperture.
> +
> +  Otherwise, the intersection of the memory space descriptor is calculated with
> +  the aperture. If the intersection is the empty set (no overlap), no action is
> +  taken; the memory space descriptor is compatible with the aperture.
> +
> +  Otherwise, the type of the descriptor is investigated again. If the type is
> +  EfiGcdMemoryTypeNonExistent (representing a gap, or a genuine descriptor with
> +  such a type), then an attempt is made to add the intersection as MMIO space
> +  to the GCD memory space map, with the specified capabilities. This ensures
> +  continuity for the aperture, and the descriptor is deemed compatible with the
> +  aperture.
> +
> +  Otherwise, the memory space descriptor is incompatible with the MMIO
> +  aperture.
> +
> +  @param[in] Base         Base address of the aperture.
> +  @param[in] Length       Length of the aperture.
> +  @param[in] Capabilities Capabilities required by the aperture.
> +  @param[in] Descriptor   The descriptor to ensure compatibility with the
> +                          aperture for.
> +
> +  @retval EFI_SUCCESS            The descriptor is compatible. The GCD memory
> +                                 space map may have been updated, for
> +                                 continuity within the aperture.
> +  @retval EFI_INVALID_PARAMETER  The descriptor is incompatible.
> +  @return                        Error codes from gDS->AddMemorySpace().
> +**/
> +EFI_STATUS
> +IntersectMemoryDescriptor (
> +  IN  UINT64                                Base,
> +  IN  UINT64                                Length,
> +  IN  UINT64                                Capabilities,
> +  IN  CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor
> +  )
> +{
> +  UINT64                                    IntersectionBase;
> +  UINT64                                    IntersectionEnd;
> +  EFI_STATUS                                Status;
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo &&
> +      (Descriptor->Capabilities & Capabilities) == Capabilities) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  IntersectionBase = MAX (Base, Descriptor->BaseAddress);
> +  IntersectionEnd = MIN (Base + Length,
> +                      Descriptor->BaseAddress + Descriptor->Length);
> +  if (IntersectionBase >= IntersectionEnd) {
> +    //
> +    // The descriptor and the aperture don't overlap.
> +    //
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> +    Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> +                    IntersectionBase, IntersectionEnd - IntersectionBase,
> +                    Capabilities);
> +
> +    DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE,
> +      "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__,
> +      IntersectionBase, IntersectionEnd, Status));
> +    return Status;
> +  }
> +
> +  DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflicts "
> +    "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__,
> +    Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length,
> +    (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities,
> +    Base, Base + Length, Capabilities));
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +/**
> +  Add MMIO space to GCD.
> +  The routine checks the GCD database and only adds those which are
> +  not added in the specified range to GCD.
> +
> +  @param Base         Base address of the MMIO space.
> +  @param Length       Length of the MMIO space.
> +  @param Capabilities Capabilities of the MMIO space.
> +
> +  @retval EFI_SUCCES The MMIO space was added successfully.
> +**/
> +EFI_STATUS
> +AddMemoryMappedIoSpace (
> +  IN  UINT64                            Base,
> +  IN  UINT64                            Length,
> +  IN  UINT64                            Capabilities
> +  )
> +{
> +  EFI_STATUS                            Status;
> +  UINTN                                 Index;
> +  UINTN                                 NumberOfDescriptors;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       *MemorySpaceMap;
> +
> +  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n",
> +      gEfiCallerBaseName, __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  for (Index = 0; Index < NumberOfDescriptors; Index++) {
> +    Status = IntersectMemoryDescriptor (Base, Length, Capabilities,
> +               &MemorySpaceMap[Index]);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeMemorySpaceMap;
> +    }
> +  }
> +
> +  DEBUG_CODE (
> +    //
> +    // Make sure there are adjacent descriptors covering [Base, Base + Length).
> +    // It is possible that they have not been merged; merging can be prevented
> +    // by allocation and different capabilities.
> +    //
> +    UINT64                          CheckBase;
> +    EFI_STATUS                      CheckStatus;
> +    EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor;
> +
> +    for (CheckBase = Base;
> +         CheckBase < Base + Length;
> +         CheckBase = Descriptor.BaseAddress + Descriptor.Length) {
> +      CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor);
> +      ASSERT_EFI_ERROR (CheckStatus);
> +      ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo);
> +      ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities);
> +    }
> +    );
> +
> +FreeMemorySpaceMap:
> +  FreePool (MemorySpaceMap);
> +
> +  return Status;
> +}
>  
>  /**
>    Initialize the state information for the CPU Architectural Protocol.
> 

Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

[Fan, Jeff]

Laszlo,

For 1), I will update this typo when pushing the serial of patches. Thanks!

For2), I will do clean up on EFI_D to DEBUG_ in CpuDxe driver after Jiewen & my patches pushed.

Thanks!
Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, February 21, 2017 6:57 PM
To: Fan, Jeff; edk2-devel@ml01.01.org
Cc: Yao, Jiewen; Tian, Feng; Kinney, Michael D
Subject: Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge

I have two suggestions for this patch:

On 02/21/17 04:08, Jeff Fan wrote:
> Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from 
> MdeModulePkg\Bus\Pci\PciHostBridgeDxe\PciHostBridge.c.

(1) One of the two "AddMemoryMappedIoSpace" strings should be replaced with "IntersectMemoryDescriptor".

This can be done when you commit / push the patch; no need to repost just because of it.

(2) This is more of a "followup" suggestion: since this is new code, I recommend that the EFI_D_ macros be replced with DEBUG_ macros.

However, I see value in keeping the first patch as a pristine copy from the PciHostBridgeDxe driver, so I don't recommend doing the above change in patch #1. I think it can be patch #3, or a completely separate patch (later).

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

Thanks!
Laszlo

> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=390
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c | 148 
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c 
> index 9fb6d76..925c50f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -887,6 +887,154 @@ IdleLoopEventCallback (
>    CpuSleep ();
>  }
>  
> +/**
> +  Ensure the compatibility of a memory space descriptor with the MMIO aperture.
> +
> +  The memory space descriptor can come from the GCD memory space map, 
> + or it can  represent a gap between two neighboring memory space 
> + descriptors. In the  latter case, the GcdMemoryType field is 
> + expected to be  EfiGcdMemoryTypeNonExistent.
> +
> +  If the memory space descriptor already has type  
> + EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset 
> + of the  required capabilities, then no action is taken -- it is by 
> + definition  compatible with the aperture.
> +
> +  Otherwise, the intersection of the memory space descriptor is 
> + calculated with  the aperture. If the intersection is the empty set 
> + (no overlap), no action is  taken; the memory space descriptor is compatible with the aperture.
> +
> +  Otherwise, the type of the descriptor is investigated again. If the 
> + type is  EfiGcdMemoryTypeNonExistent (representing a gap, or a 
> + genuine descriptor with  such a type), then an attempt is made to 
> + add the intersection as MMIO space  to the GCD memory space map, 
> + with the specified capabilities. This ensures  continuity for the 
> + aperture, and the descriptor is deemed compatible with the  aperture.
> +
> +  Otherwise, the memory space descriptor is incompatible with the 
> + MMIO  aperture.
> +
> +  @param[in] Base         Base address of the aperture.
> +  @param[in] Length       Length of the aperture.
> +  @param[in] Capabilities Capabilities required by the aperture.
> +  @param[in] Descriptor   The descriptor to ensure compatibility with the
> +                          aperture for.
> +
> +  @retval EFI_SUCCESS            The descriptor is compatible. The GCD memory
> +                                 space map may have been updated, for
> +                                 continuity within the aperture.
> +  @retval EFI_INVALID_PARAMETER  The descriptor is incompatible.
> +  @return                        Error codes from gDS->AddMemorySpace().
> +**/
> +EFI_STATUS
> +IntersectMemoryDescriptor (
> +  IN  UINT64                                Base,
> +  IN  UINT64                                Length,
> +  IN  UINT64                                Capabilities,
> +  IN  CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor
> +  )
> +{
> +  UINT64                                    IntersectionBase;
> +  UINT64                                    IntersectionEnd;
> +  EFI_STATUS                                Status;
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo &&
> +      (Descriptor->Capabilities & Capabilities) == Capabilities) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  IntersectionBase = MAX (Base, Descriptor->BaseAddress);  
> + IntersectionEnd = MIN (Base + Length,
> +                      Descriptor->BaseAddress + Descriptor->Length);  
> + if (IntersectionBase >= IntersectionEnd) {
> +    //
> +    // The descriptor and the aperture don't overlap.
> +    //
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> +    Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> +                    IntersectionBase, IntersectionEnd - IntersectionBase,
> +                    Capabilities);
> +
> +    DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE,
> +      "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__,
> +      IntersectionBase, IntersectionEnd, Status));
> +    return Status;
> +  }
> +
> +  DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflicts "
> +    "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__,
> +    Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length,
> +    (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities,
> +    Base, Base + Length, Capabilities));
> +  return EFI_INVALID_PARAMETER;
> +}
> +
> +/**
> +  Add MMIO space to GCD.
> +  The routine checks the GCD database and only adds those which are
> +  not added in the specified range to GCD.
> +
> +  @param Base         Base address of the MMIO space.
> +  @param Length       Length of the MMIO space.
> +  @param Capabilities Capabilities of the MMIO space.
> +
> +  @retval EFI_SUCCES The MMIO space was added successfully.
> +**/
> +EFI_STATUS
> +AddMemoryMappedIoSpace (
> +  IN  UINT64                            Base,
> +  IN  UINT64                            Length,
> +  IN  UINT64                            Capabilities
> +  )
> +{
> +  EFI_STATUS                            Status;
> +  UINTN                                 Index;
> +  UINTN                                 NumberOfDescriptors;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       *MemorySpaceMap;
> +
> +  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, 
> + &MemorySpaceMap);  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n",
> +      gEfiCallerBaseName, __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  for (Index = 0; Index < NumberOfDescriptors; Index++) {
> +    Status = IntersectMemoryDescriptor (Base, Length, Capabilities,
> +               &MemorySpaceMap[Index]);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeMemorySpaceMap;
> +    }
> +  }
> +
> +  DEBUG_CODE (
> +    //
> +    // Make sure there are adjacent descriptors covering [Base, Base + Length).
> +    // It is possible that they have not been merged; merging can be prevented
> +    // by allocation and different capabilities.
> +    //
> +    UINT64                          CheckBase;
> +    EFI_STATUS                      CheckStatus;
> +    EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor;
> +
> +    for (CheckBase = Base;
> +         CheckBase < Base + Length;
> +         CheckBase = Descriptor.BaseAddress + Descriptor.Length) {
> +      CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor);
> +      ASSERT_EFI_ERROR (CheckStatus);
> +      ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo);
> +      ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities);
> +    }
> +    );
> +
> +FreeMemorySpaceMap:
> +  FreePool (MemorySpaceMap);
> +
> +  return Status;
> +}
>  
>  /**
>    Initialize the state information for the CPU Architectural Protocol.
> 

[PATCH v2 2/2] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in GCD

[Jeff Fan]

Local APIC memory mapped space should be added into GCD and be allocated.
Otherwise, UEFI firmware cannot get correct memory map for it. For example,
SMM profile feature needs to get the completed MMIO map to protect them.

v2:
  Consume AddMemoryMappedIoSpace() to handle the case that Local APIC
  memory space has already been added before.

https://bugzilla.tianocore.org/show_bug.cgi?id=390

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuDxe.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 925c50f..84d5f78 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -1037,6 +1037,36 @@ FreeMemorySpaceMap:
 }
 
 /**
+  Add and allocate CPU local APIC memory mapped space. 
+
+  @param[in]ImageHandle     Image handle this driver.
+
+**/
+VOID
+AddLocalApicMemorySpace (
+  IN EFI_HANDLE               ImageHandle
+  )
+{
+  EFI_STATUS              Status;
+  EFI_PHYSICAL_ADDRESS    BaseAddress;
+
+  BaseAddress = (EFI_PHYSICAL_ADDRESS) GetLocalApicBaseAddress();
+  Status = AddMemoryMappedIoSpace (BaseAddress, SIZE_4KB, EFI_MEMORY_UC);
+  ASSERT_EFI_ERROR (Status);
+
+  Status = gDS->AllocateMemorySpace (
+                  EfiGcdAllocateAddress,
+                  EfiGcdMemoryTypeMemoryMappedIo,
+                  0,
+                  SIZE_4KB,
+                  &BaseAddress,
+                  ImageHandle,
+                  NULL
+                  );
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
   Initialize the state information for the CPU Architectural Protocol.
 
   @param ImageHandle     Image handle this driver.
@@ -1095,6 +1125,11 @@ InitializeCpu (
   RefreshGcdMemoryAttributes ();
 
   //
+  // Add and allocate local APIC memory mapped space
+  //
+  AddLocalApicMemorySpace (ImageHandle);
+
+  //
   // Setup a callback for idle events
   //
   Status = gBS->CreateEventEx (
-- 
2.9.3.windows.2