* [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
@ 2017-02-20 8:40 Jeff Fan
2017-02-20 9:11 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Fan @ 2017-02-20 8:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Feng Tian, Michael D Kinney
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);
+
+ 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 (
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
2017-02-20 8:40 [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS Jeff Fan
@ 2017-02-20 9:11 ` Laszlo Ersek
2017-02-21 1:33 ` Fan, Jeff
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-02-20 9:11 UTC (permalink / raw)
To: Jeff Fan, edk2-devel; +Cc: Michael D Kinney, Feng Tian, Jiewen Yao
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 (
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
2017-02-20 9:11 ` Laszlo Ersek
@ 2017-02-21 1:33 ` Fan, Jeff
2017-02-21 8:32 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Fan, Jeff @ 2017-02-21 1:33 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@ml01.01.org
Cc: Kinney, Michael D, Tian, Feng, Yao, Jiewen
Laszlo,
I added my feedback as below. Thanks!
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Monday, February 20, 2017 5:11 PM
To: Fan, Jeff; edk2-devel@ml01.01.org
Cc: Kinney, Michael D; Tian, Feng; Yao, Jiewen
Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
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).
[Jeff] I agree. Platform Pei could use resource HOB to declare this range. I may sync the code from 6474f1f156ee to CpuDxe.
(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.
[Jeff] Actually, I don't think there is other module(except for Cpu Driver) allocating such range. If other module does it, we need to think if it does make sense and how to fix it.
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 (
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
2017-02-21 1:33 ` Fan, Jeff
@ 2017-02-21 8:32 ` Laszlo Ersek
0 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2017-02-21 8:32 UTC (permalink / raw)
To: Fan, Jeff, edk2-devel@ml01.01.org
Cc: Kinney, Michael D, Tian, Feng, Yao, Jiewen
On 02/21/17 02:33, Fan, Jeff wrote:
> Laszlo,
>
> I added my feedback as below. Thanks!
>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, February 20, 2017 5:11 PM
> To: Fan, Jeff; edk2-devel@ml01.01.org
> Cc: Kinney, Michael D; Tian, Feng; Yao, Jiewen
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS
>
> 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).
> [Jeff] I agree. Platform Pei could use resource HOB to declare this range. I may sync the code from 6474f1f156ee to CpuDxe.
Thanks for considering that!
> (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.
> [Jeff] Actually, I don't think there is other module(except for Cpu Driver) allocating such range. If other module does it, we need to think if it does make sense and how to fix it.
I wonder if it makes sense for CpuDxe to "own" the LAPIC address range.
If CpuDxe can be said to "own" the LAPIC, then allocating the range in
CpuDxe certainly makes sense. However, given that LocalApicLib is a
widely usable (and widely used) library class, I wonder if such an
"ownership" can be established.
... Either way, I think enforcing success for the MMIO range allocation
is much less problematic than enforcing success for the MMIO range
addition. So, from an OVMF platform perspective at least, I guess the
second ASSERT_EFI_ERROR() might even stay as-is.
Thank you!
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 (
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-21 8:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-20 8:40 [PATCH] UefiCpuPkg/CpuDxe: Add Local APIC memory mapped space in gDS Jeff Fan
2017-02-20 9:11 ` Laszlo Ersek
2017-02-21 1:33 ` Fan, Jeff
2017-02-21 8:32 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox