Re: [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS

[Laszlo Ersek <lersek@redhat.com>]

Hi Jeff,

On 02/20/17 09:40, Jeff Fan wrote:
> Local APIC memory mapped space should be added into gDS 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.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=390
> 
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 9fb6d76..71a08cd 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -887,6 +887,40 @@ IdleLoopEventCallback (
>    CpuSleep ();
>  }
>  
> +/**
> +  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 = gDS->AddMemorySpace (
> +                  EfiGcdMemoryTypeMemoryMappedIo, 
> +                  BaseAddress,
> +                  SIZE_4KB,
> +                  EFI_MEMORY_UC
> +                  );
> +  ASSERT_EFI_ERROR (Status);

(1) This would break OVMF:

> Loading driver at 0x0007F510000 EntryPoint=0x0007F51027B CpuDxe.efi
> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 7EE08498
> InstallProtocolInterface: [EfiCpuArchProtocol] 7F522480
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>   Flushing GCD
>
> ASSERT_EFI_ERROR (Status = Access Denied)
> ASSERT .../UefiCpuPkg/CpuDxe/CpuDxe.c(891): !EFI_ERROR (Status)

The reason for the assertion failure is that we add an uncacheable MMIO
resource descriptor HOB for the LAPIC range earlier, in
OvmfPkg/PlatformPei. When DXE starts up, it initializes the GCD memory
space accordingly, hence the above addition failure.

There was a similar issue in PciHostBridgeDxe: originally it tried to
add uncacheable MMIO space for the PCI bridges' apertures,
unconditionally. The solution was to check the GCD memory space map, and:
- for any existing, overlapping entries, check if their attributes were
compatible with UC,
- missing gaps were filled with new additions.

Please see commit 6474f1f156ee ("MdeModulePkg/PciHostBridge: Don't
assume resources are fully NonExistent", 2016-02-25).

(2) After the memory space addition, the allocation should be attempted,
I agree. But for that too, if it fails, that shouldn't be a fatal error.

Thanks
Laszlo

> +
> +  Status = gDS->AllocateMemorySpace (
> +                  EfiGcdAllocateAddress,
> +                  EfiGcdMemoryTypeMemoryMappedIo,
> +                  0,
> +                  SIZE_4KB,
> +                  &BaseAddress,
> +                  ImageHandle,
> +                  NULL
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +}
>  
>  /**
>    Initialize the state information for the CPU Architectural Protocol.
> @@ -947,6 +981,11 @@ InitializeCpu (
>    RefreshGcdMemoryAttributes ();
>  
>    //
> +  // Add and allocate local APIC memory mapped space
> +  //
> +  AddLocalApicMemorySpace (ImageHandle);
> +
> +  //
>    // Setup a callback for idle events
>    //
>    Status = gBS->CreateEventEx (
>