Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
I'm using the OVMF library, haven't try the AMD library:). > -Original Message- > From: Gerd Hoffmann > Sent: Wednesday, April 17, 2024 3:04 PM > To: Wu, Jiaxin > Cc: thomas.lenda...@amd.com; devel@edk2.groups.io; Ni, Ray > ; Zeng, Star ; Ard Biesheuvel > ; Yao, Jiewen ; Kumar, > Rahul R > Subject: Re: [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library > instance for OVMF > > On Tue, Apr 16, 2024 at 11:34:00AM +, Wu, Jiaxin wrote: > > Hi Gerd, > > > > > Is the SmmRelocationLib approach supposed to work with mixed mode > > > firmware where PEI is running in ia32 mode and dxe/smm is running > > > in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)? > > > > Yes, I passed the test on the both OvmfPkgIa32X64 & OvmfPkgX64 for SMM > support. > > The OVMF library I assume? > Did you try the AMD library too? > > > It does has the problem on OvmfPkgIa32 for smm support (same as > > master). I did quick check, it's not only the CpuSaveState->x86 or > > CpuSaveState->x64 structure issue, but also has some problem to > > handler the smi hook return. We can handle this problem in another > > topic. > > Yes, fixing ia32 can be done separately. > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117902): https://edk2.groups.io/g/devel/message/117902 Mute This Topic: https://groups.io/mt/105441992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
On Tue, Apr 16, 2024 at 11:34:00AM +, Wu, Jiaxin wrote: > Hi Gerd, > > > Is the SmmRelocationLib approach supposed to work with mixed mode > > firmware where PEI is running in ia32 mode and dxe/smm is running > > in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)? > > Yes, I passed the test on the both OvmfPkgIa32X64 & OvmfPkgX64 for SMM > support. The OVMF library I assume? Did you try the AMD library too? > It does has the problem on OvmfPkgIa32 for smm support (same as > master). I did quick check, it's not only the CpuSaveState->x86 or > CpuSaveState->x64 structure issue, but also has some problem to > handler the smi hook return. We can handle this problem in another > topic. Yes, fixing ia32 can be done separately. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117897): https://edk2.groups.io/g/devel/message/117897 Mute This Topic: https://groups.io/mt/105441992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
Hi Gerd, > > I don't the background why AMD and OVMF has such difference. Maybe > OVFM doesn't not support the MSR "EFER_ADDRESS". > > It surely does, it's a rather essential MSR for x64 CPUs. > > > > The SmBase configuration for OVMF looks suspicious to me. I'm > wondering > > > whenever the OVMF code actually works in Ia32 builds ... > > Tested OvmfPkg/OvmfPkgIa32.dsc with SMM_REQUIRE == TRUE (master > branch). > Doesn't boot. The difference above (where the OVMF code does not > consider the 32bit case) could very well explain why 32bit support is > broken. Switching to the AMD code might actually fix that. > > Is the SmmRelocationLib approach supposed to work with mixed mode > firmware where PEI is running in ia32 mode and dxe/smm is running > in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)? > Yes, I passed the test on the both OvmfPkgIa32X64 & OvmfPkgX64 for SMM support. It does has the problem on OvmfPkgIa32 for smm support (same as master). I did quick check, it's not only the CpuSaveState->x86 or CpuSaveState->x64 structure issue, but also has some problem to handler the smi hook return. We can handle this problem in another topic. Thanks, Jiaxin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117878): https://edk2.groups.io/g/devel/message/117878 Mute This Topic: https://groups.io/mt/105441992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
Hi,
> > > 2) Existing SmBase configuration is different between the AMD & OVMF.
> > > OVMF:
> > > AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > >
> > > AMD:
> > > if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
> > > CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > > } else {
> > > CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > > }
> I don't the background why AMD and OVMF has such difference. Maybe OVFM
> doesn't not support the MSR "EFER_ADDRESS".
It surely does, it's a rather essential MSR for x64 CPUs.
> > The SmBase configuration for OVMF looks suspicious to me. I'm wondering
> > whenever the OVMF code actually works in Ia32 builds ...
Tested OvmfPkg/OvmfPkgIa32.dsc with SMM_REQUIRE == TRUE (master branch).
Doesn't boot. The difference above (where the OVMF code does not
consider the 32bit case) could very well explain why 32bit support is
broken. Switching to the AMD code might actually fix that.
Is the SmmRelocationLib approach supposed to work with mixed mode
firmware where PEI is running in ia32 mode and dxe/smm is running
in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)?
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117877): https://edk2.groups.io/g/devel/message/117877
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
>
> On Mon, Apr 15, 2024 at 01:04:58PM +, Wu, Jiaxin wrote:
> > Hi Gred,
> >
> > Because:
> > 1) The mode of the CPU check is different between the AMD & OVMF.
> > OVMF:
> > CpuSaveState->x86.SMMRevId & 0X
> >
> > AMD:
> > LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA
> >
> > 2) Existing SmBase configuration is different between the AMD & OVMF.
> > OVMF:
> > AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> >
> > AMD:
> > if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
> > CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > } else {
> > CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > }
> >
> > This series patch won't change the existing implementation code logic, so,
> > we
> need override one version for OVMF.
>
> The real question is why do these differences exist and are they
> actually needed.
>
> I'd expect the CPU mode check return identical results.
I don't the background why AMD and OVMF has such difference. Maybe OVFM doesn't
not support the MSR "EFER_ADDRESS".
>
> The SmBase configuration for OVMF looks suspicious to me. I'm wondering
> whenever the OVMF code actually works in Ia32 builds ...
>
Yes, I will fix it in the next version. Currently, please build it with
SMM_REQUIRE == TRUE. In the next version, I will make sure all pass the check.
> take care,
> Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117874): https://edk2.groups.io/g/devel/message/117874
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
On Mon, Apr 15, 2024 at 01:04:58PM +, Wu, Jiaxin wrote:
> Hi Gred,
>
> Because:
> 1) The mode of the CPU check is different between the AMD & OVMF.
> OVMF:
> CpuSaveState->x86.SMMRevId & 0X
>
> AMD:
> LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA
>
> 2) Existing SmBase configuration is different between the AMD & OVMF.
> OVMF:
> AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
>
> AMD:
> if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
> CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> } else {
> CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> }
>
> This series patch won't change the existing implementation code logic, so, we
> need override one version for OVMF.
The real question is why do these differences exist and are they
actually needed.
I'd expect the CPU mode check return identical results.
The SmBase configuration for OVMF looks suspicious to me. I'm wondering
whenever the OVMF code actually works in Ia32 builds ...
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117853): https://edk2.groups.io/g/devel/message/117853
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
Hi Gred,
Because:
1) The mode of the CPU check is different between the AMD & OVMF.
OVMF:
CpuSaveState->x86.SMMRevId & 0X
AMD:
LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA
2) Existing SmBase configuration is different between the AMD & OVMF.
OVMF:
AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
AMD:
if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
} else {
CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
}
This series patch won't change the existing implementation code logic, so, we
need override one version for OVMF.
+AMD feature owner
Thanks,
Jiaxin
> -Original Message-
> From: Gerd Hoffmann
> Sent: Thursday, April 11, 2024 3:11 PM
> To: Wu, Jiaxin
> Cc: devel@edk2.groups.io; Ni, Ray ; Zeng, Star
> ; Ard Biesheuvel ; Yao,
> Jiewen ; Kumar, Rahul R
> Subject: Re: [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library
> instance for OVMF
>
> On Wed, Apr 10, 2024 at 09:57:14PM +0800, Jiaxin Wu wrote:
> > Due to the definition difference of SMRAM Save State,
> > SmmBase config in SMRAM Save State for OVMF is also different.
> >
> > This patch provides the OvmfSmmRelocationLib library instance
> > to handle the SMRAM Save State difference.
>
> Why ovmf needs its own version? Patch #4 adds an AMD version, and given
> that KVM uses the AMD smram layout that library should work for OVMF
> too, no?
>
> take care,
> Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117774): https://edk2.groups.io/g/devel/message/117774
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
On Wed, Apr 10, 2024 at 09:57:14PM +0800, Jiaxin Wu wrote: > Due to the definition difference of SMRAM Save State, > SmmBase config in SMRAM Save State for OVMF is also different. > > This patch provides the OvmfSmmRelocationLib library instance > to handle the SMRAM Save State difference. Why ovmf needs its own version? Patch #4 adds an AMD version, and given that KVM uses the AMD smram layout that library should work for OVMF too, no? take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117618): https://edk2.groups.io/g/devel/message/117618 Mute This Topic: https://groups.io/mt/105441992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF
No. Please do not create a OVMF specific lib instance in UefiCpuPkg.
Thanks,
Ray
From: Wu, Jiaxin
Sent: Wednesday, April 10, 2024 21:57
To: devel@edk2.groups.io
Cc: Ni, Ray ; Zeng, Star ; Ard
Biesheuvel ; Yao, Jiewen ;
Gerd Hoffmann ; Kumar, Rahul R
Subject: [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for
OVMF
Due to the definition difference of SMRAM Save State,
SmmBase config in SMRAM Save State for OVMF is also different.
This patch provides the OvmfSmmRelocationLib library instance
to handle the SMRAM Save State difference.
Cc: Ray Ni
Cc: Zeng Star
Cc: Ard Biesheuvel
Cc: Jiewen Yao
Cc: Gerd Hoffmann
Cc: Rahul Kumar
Signed-off-by: Jiaxin Wu
---
.../SmmRelocationLib/OvmfSmmRelocationLib.inf | 61
.../SmmRelocationLib/OvmfSmramSaveStateConfig.c| 107 +
2 files changed, 168 insertions(+)
create mode 100644 UefiCpuPkg/Library/SmmRelocationLib/OvmfSmmRelocationLib.inf
create mode 100644
UefiCpuPkg/Library/SmmRelocationLib/OvmfSmramSaveStateConfig.c
diff --git a/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmmRelocationLib.inf
b/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmmRelocationLib.inf
new file mode 100644
index 00..eba1129ac2
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmmRelocationLib.inf
@@ -0,0 +1,61 @@
+## @file
+# SMM Relocation Lib for each processor.
+#
+# This Lib produces the SMM_BASE_HOB in HOB database which tells
+# the PiSmmCpuDxeSmm driver (runs at a later phase) about the new
+# SMBASE for each processor. PiSmmCpuDxeSmm driver installs the
+# SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for processor
+# Index.
+#
+# Copyright (c) 2024, Intel Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION= 0x00010005
+ BASE_NAME = SmmRelocationLib
+ FILE_GUID = 51834F51-CCE0-4743-B553-935D0C8A53FF
+ MODULE_TYPE= PEIM
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SmmRelocationLib
+
+[Sources]
+ InternalSmmRelocationLib.h
+ OvmfSmramSaveStateConfig.c
+ SmmRelocationLib.c
+
+[Sources.Ia32]
+ Ia32/Semaphore.c
+ Ia32/SmmInit.nasm
+
+[Sources.X64]
+ X64/Semaphore.c
+ X64/SmmInit.nasm
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
+ CpuExceptionHandlerLib
+ DebugLib
+ HobLib
+ LocalApicLib
+ MemoryAllocationLib
+ PcdLib
+ PeiServicesLib
+
+[Guids]
+ gSmmBaseHobGuid ## HOB ALWAYS_PRODUCED
+ gEfiSmmSmramMemoryGuid## CONSUMES
+
+[Pcd]
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize ## CONSUMES
+
+[FeaturePcd]
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport##
CONSUMES
diff --git a/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmramSaveStateConfig.c
b/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmramSaveStateConfig.c
new file mode 100644
index 00..505b1d694a
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmRelocationLib/OvmfSmramSaveStateConfig.c
@@ -0,0 +1,107 @@
+/** @file
+ Config SMRAM Save State for SmmBases Relocation.
+
+ Copyright (c) 2024, Intel Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include "InternalSmmRelocationLib.h"
+#include
+
+/**
+ This function configures the SmBase on the currently executing CPU.
+
+ @param[in] CpuIndex The index of the CPU.
+ @param[in,out] CpuState Pointer to SMRAM Save State Map for the
+ currently executing CPU. On out, SmBase
is
+ updated to the new value.
+
+**/
+VOID
+EFIAPI
+ConfigureSmBase (
+ IN UINTN CpuIndex,
+ IN OUT SMRAM_SAVE_STATE_MAP *CpuState
+ )
+{
+ AMD_SMRAM_SAVE_STATE_MAP *CpuSaveState;
+
+ CpuSaveState = (AMD_SMRAM_SAVE_STATE_MAP *)CpuState;
+
+ if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
+CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
+ } else {
+CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
+ }
+}
+
+/**
+ This function updates the SMRAM save state on the currently executing CPU
+ to resume execution at a specific address after an RSM instruction. This
+ function must evaluate the SMRAM save state to determine the execution mode
+ the RSM instruction resumes and update the resume execution address with
+ either NewInstructionPointer32 or NewInstructionPoint. The auto HALT restart
+ flag in the SMRAM save state must always be cleared. This function returns
+ the value of the instruction pointer from the SMRAM save state that was
+ replaced. If this function returns 0, then the SMRAM save state was not
+ modified.
+
+ This
