[edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[Jacque Lin via groups.io]

Remove checking SMM Rev ID in AMD Save State lib when reading Save
State Register EFI_MM_SAVE_STATE_REGISTER_IO.
For AMD, it is not necessary to check SmmRevId when reading Save State
Register EFI_MM_SAVE_STATE_REGISTER_IO.

Cc: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
Cc: Abner Chang <abner.chang@amd.com>
Signed-off-by: Jacque Lin <hsienchieh.lin@amd.com>
---
 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
index 3315a6cc44..c4bf6ad4bb 100644
--- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
+++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
@@ -102,7 +102,6 @@ MmSaveStateReadRegister (
   OUT VOID                        *Buffer

   )

 {

-  UINT32                     SmmRevId;

   EFI_MM_SAVE_STATE_IO_INFO  *IoInfo;

   AMD_SMRAM_SAVE_STATE_MAP   *CpuSaveState;

   UINT8                      DataWidth;

@@ -124,18 +123,6 @@ MmSaveStateReadRegister (
 

   // Check for special EFI_MM_SAVE_STATE_REGISTER_IO

   if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {

-    //

-    // Get SMM Revision ID

-    //

-    MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);

-

-    //

-    // See if the CPU supports the IOMisc register in the save state

-    //

-    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {

-      return EFI_NOT_FOUND;

-    }

-

     // Check if IO Restart Dword [IO Trap] is valid or not using bit 1.

     if (!(CpuSaveState->x64.IO_DWord & 0x02u)) {

       return EFI_NOT_FOUND;

-- 
2.36.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110289): https://edk2.groups.io/g/devel/message/110289
Mute This Topic: https://groups.io/mt/102269908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[Chang, Abner via groups.io]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

Acked-by: Abner Chang <abner.chang@amd.com>

Still need Abdul's RB.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110290): https://edk2.groups.io/g/devel/message/110290
Mute This Topic: https://groups.io/mt/102269908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 928 bytes --]

Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[Laszlo Ersek]

+Gerd, +Paolo

On 10/30/23 07:12, Jacque Lin via groups.io wrote:
> Remove checking SMM Rev ID in AMD Save State lib when reading Save
> State Register EFI_MM_SAVE_STATE_REGISTER_IO.
> For AMD, it is not necessary to check SmmRevId when reading Save State
> Register EFI_MM_SAVE_STATE_REGISTER_IO.
>
> Cc: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Signed-off-by: Jacque Lin <hsienchieh.lin@amd.com>
> ---
>  UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> index 3315a6cc44..c4bf6ad4bb 100644
> --- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> +++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> @@ -102,7 +102,6 @@ MmSaveStateReadRegister (
>    OUT VOID                        *Buffer
>
>    )
>
>  {
>
> -  UINT32                     SmmRevId;
>
>    EFI_MM_SAVE_STATE_IO_INFO  *IoInfo;
>
>    AMD_SMRAM_SAVE_STATE_MAP   *CpuSaveState;
>
>    UINT8                      DataWidth;
>
> @@ -124,18 +123,6 @@ MmSaveStateReadRegister (
>
>
>    // Check for special EFI_MM_SAVE_STATE_REGISTER_IO
>
>    if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {
>
> -    //
>
> -    // Get SMM Revision ID
>
> -    //
>
> -    MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);
>
> -
>
> -    //
>
> -    // See if the CPU supports the IOMisc register in the save state
>
> -    //
>
> -    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
>
> -      return EFI_NOT_FOUND;
>
> -    }
>
> -
>
>      // Check if IO Restart Dword [IO Trap] is valid or not using bit 1.
>
>      if (!(CpuSaveState->x64.IO_DWord & 0x02u)) {
>
>        return EFI_NOT_FOUND;
>

I still don't understand.

Are you saying that the

  SmmRevId < AMD_SMM_MIN_REV_ID_X64

check will always evaluate to FALSE, and so the "return EFI_NOT_FOUND"
branch is dead code?

If that's the case, then the patch seems right, but *why* is SmmRevId
always greater than or equal to AMD_SMM_MIN_REV_ID_X64?

SmmRevId is used to tell apart x86 from x64 (i.e., to distinguish the
two formats of the save state map). Is the intent of this patch to
remove 32-bit (x86) support?

That makes me uncomfortable, because it could break SMM support in
*IA32* OVMF. Note that commit f2188fe5d155 ("OvmfPkg: Uses
MmSaveStateLib library", 2023-07-03) also updated
"OvmfPkg/OvmfPkgIa32.dsc".

In fact, I'm worried that the conversion of OVMF to MmSaveStateLib may
have been incorrect. Note that commit f2188fe5d155 removed the following
comment from "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c":

> //
> // No support for I/O restart
> //

Furthermore, commit f2188fe5d155 removed the following functions:
GetRegisterIndex(), ReadSaveStateRegisterByIndex(),
SmmCpuFeaturesReadSaveStateRegister(),
SmmCpuFeaturesWriteSaveStateRegister().

In particular, the latter two functions contained comments and code
like:

>   //
>   // Check for special EFI_SMM_SAVE_STATE_REGISTER_IO
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_IO) {
>     return EFI_NOT_FOUND;
>   }

and

>   //
>   // Writes to EFI_SMM_SAVE_STATE_REGISTER_IO are not supported
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_IO) {
>     return EFI_NOT_FOUND;
>   }

All of these came from Paolo's original commit 4036b4e57cce ("OvmfPkg:
SmmCpuFeaturesLib: implement SMRAM state save map access", 2015-11-30).

Consider the following commits from Paolo (including 4036b4e57cce):

> commit 86d71589c1fdb099c04a0f9e548fe868a2fef266
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:27 2015 +0000
>
>     OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg
>
>     The next patches will customize the implementation, but let's start from
>     the common version to better show the changes.

and

> commit d7e71b2925012c9706d1d044ca466173aac802a8
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:32 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits
>
>     SMRR, MTRR, and SMM Feature Control support is not needed on a virtual
>     platform.

and

> commit 4036b4e57ccef4e0fa48d8389acf6390826c2bed
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:37 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access
>
>     This implementation copies SMRAM state save map access from the
>     PiSmmCpuDxeSmm module.
>
>     The most notable change is:
>
>     - dropping support for EFI_SMM_SAVE_STATE_REGISTER_IO
>
>     - changing the implementation of EFI_SMM_SAVE_STATE_REGISTER_LMA to use
>       the SMM revision id instead of a local variable (which
>       UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c initializes from CPUID's LM
>       bit).  This accounts for QEMU's implementation of x86_64, which always
>       uses revision 0x20064 even if the LM bit is zero.

and

> commit c1fcd80bf42e6b1e91c1c742d222f1ba421b1d1d
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:42 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: customize state save map format
>
>     This adjusts the previously introduced state save map access functions, to
>     account for QEMU and KVM's 64-bit state save map following the AMD spec
>     rather than the Intel one.

Were these QEMU-specific differences considered when OVMF was migrated
to MmSaveStateLib, in commit?

Before the patches for TianoCore Bugzilla 4182 (commit range
ad7d3ace1ad4..f2188fe5d155), we used to have the following call tree,
for EFI_SMM_CPU_PROTOCOL.ReadSaveState, in OVMF:

  SmmReadSaveState()                      [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
    SmmCpuFeaturesReadSaveStateRegister() [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]
      return EFI_NOT_FOUND
      for EFI_SMM_SAVE_STATE_REGISTER_IO

Thus, the protocol member function would propagate EFI_NOT_FOUND
outwards, and ReadSaveStateRegister()
[UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c] would not be reached. In
particular, the register would not be accessed at all.

After commit f2188fe5d155 however, we have this, in OVMF:

  SmmReadSaveState()          [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
    MmSaveStateReadRegister() [UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c]

Namely:

>   // Check for special EFI_MM_SAVE_STATE_REGISTER_IO
>   if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {
>     //
>     // Get SMM Revision ID
>     //
>     MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);
>
>     //
>     // See if the CPU supports the IOMisc register in the save state
>     //
>     if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
>       return EFI_NOT_FOUND;
>     }
>
>     // Check if IO Restart Dword [IO Trap] is valid or not using bit 1.
>     if (!(CpuSaveState->x64.IO_DWord & 0x02u)) {
>       return EFI_NOT_FOUND;
>     }

So, indeed. The patches for TianoCore Bugzilla 4182 were not correct for
OVMF.

And *this* patch would make things even worse.

As Paolo's commit 4036b4e57cce above states, QEMU #defines
SMM_REVISION_ID as 0x00020064 (see
"target/i386/tcg/sysemu/smm_helper.c").

That means that, *even after* commit f2188fe5d155, the check

    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
      return EFI_NOT_FOUND;
    }

would prevent OVMF from accessing EFI_MM_SAVE_STATE_REGISTER_IO, because
AMD_SMM_MIN_REV_ID_X64 is #defined as 0x30064 in
"MdePkg/Include/Register/Amd/SmramSaveStateMap.h".

So the condition would actually evaluate to TRUE,
MmSaveStateReadRegister() would return EFI_NOT_FOUND, and
SmmReadSaveState() -- the EFI_SMM_CPU_PROTOCOL.ReadSaveState protocol
member function -- would also return EFI_NOT_FOUND.

Put differently, that particular check would maintain the consistency of
OVMF's behavior regarding EFI_MM_SAVE_STATE_REGISTER_IO, *even though*
the patches for TianoCore Bugzilla 4182 (commit range
ad7d3ace1ad4..f2188fe5d155) may have been incorrect in other aspects.

And then the *present* patch would break *that* last resort.

In short, the statement "QEMU/OVMF uses the AMD save state map" is not
accurate. In most senses, yes, but not exactly.

Therefore this patch is not acceptable for OVMF (unless you can prove
that it makes absolutely no difference for either of IA32, IA32X64, and
X64 OVMF).


... Back to the already upstream patches. Again from Paolo's original
commit 4036b4e57cce ("OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state
save map access", 2015-11-30), we used to have the following access
logic for EFI_SMM_SAVE_STATE_REGISTER_LMA, at commit ad7d3ace1ad4 (i.e.,
before the conversion), in SmmCpuFeaturesReadSaveStateRegister()
[OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]

>   //
>   // Check for special EFI_SMM_SAVE_STATE_REGISTER_LMA
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_LMA) {
>     //
>     // Only byte access is supported for this register
>     //
>     if (Width != 1) {
>       return EFI_INVALID_PARAMETER;
>     }
>
>     CpuSaveState = (QEMU_SMRAM_SAVE_STATE_MAP *)gSmst->CpuSaveState[CpuIndex];
>
>     //
>     // Check CPU mode
>     //
>     if ((CpuSaveState->x86.SMMRevId & 0xFFFF) == 0) {
>       *(UINT8 *)Buffer = 32;
>     } else {
>       *(UINT8 *)Buffer = 64;
>     }
>
>     return EFI_SUCCESS;
>   }

But now we have, in MmSaveStateReadRegister()
[UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c]:

>   // Check for special EFI_MM_SAVE_STATE_REGISTER_LMA
>   if (Register == EFI_MM_SAVE_STATE_REGISTER_LMA) {
>     // Only byte access is supported for this register
>     if (Width != 1) {
>       return EFI_INVALID_PARAMETER;
>     }
>
>     *(UINT8 *)Buffer = MmSaveStateGetRegisterLma ();
>
>     return EFI_SUCCESS;
>   }

where MmSaveStateGetRegisterLma()
[UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c] does the following:

>   UINT32  LMAValue;
>
>   LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA;
>   if (LMAValue) {
>     return EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT;
>   }
>
>   return EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT;

I don't know if this is still correct on QEMU or not, but this is not
what the original code did. The original code checked SMMRevId, as Paolo
said in the message of commit 4036b4e57cce; the new code reads the EFER
MSR.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110317): https://edk2.groups.io/g/devel/message/110317
Mute This Topic: https://groups.io/mt/102269908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[Attar, AbdulLateef (Abdul Lateef) via groups.io]

[Public]

Hi Laszlo and @Lin, Jacque
        Please find my response inline.
Thanks
AbduL
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Monday, October 30, 2023 7:34 PM
To: devel@edk2.groups.io; Lin, Jacque <HsienChieh.Lin@amd.com>
Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Chang, Abner <Abner.Chang@amd.com>; Gerd Hoffmann <kraxel@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


+Gerd, +Paolo

On 10/30/23 07:12, Jacque Lin via groups.io wrote:
> Remove checking SMM Rev ID in AMD Save State lib when reading Save
> State Register EFI_MM_SAVE_STATE_REGISTER_IO.
> For AMD, it is not necessary to check SmmRevId when reading Save State
> Register EFI_MM_SAVE_STATE_REGISTER_IO.
>
> Cc: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Signed-off-by: Jacque Lin <hsienchieh.lin@amd.com>
> ---
>  UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> index 3315a6cc44..c4bf6ad4bb 100644
> --- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> +++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> @@ -102,7 +102,6 @@ MmSaveStateReadRegister (
>    OUT VOID                        *Buffer
>
>    )
>
>  {
>
> -  UINT32                     SmmRevId;
>
>    EFI_MM_SAVE_STATE_IO_INFO  *IoInfo;
>
>    AMD_SMRAM_SAVE_STATE_MAP   *CpuSaveState;
>
>    UINT8                      DataWidth;
>
> @@ -124,18 +123,6 @@ MmSaveStateReadRegister (
>
>
>    // Check for special EFI_MM_SAVE_STATE_REGISTER_IO
>
>    if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {
>
> -    //
>
> -    // Get SMM Revision ID
>
> -    //
>
> -    MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);
>
> -
>
> -    //
>
> -    // See if the CPU supports the IOMisc register in the save state
>
> -    //
>
> -    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
>
> -      return EFI_NOT_FOUND;
>
> -    }
>
> -
>
>      // Check if IO Restart Dword [IO Trap] is valid or not using bit 1.
>
>      if (!(CpuSaveState->x64.IO_DWord & 0x02u)) {
>
>        return EFI_NOT_FOUND;
>

I still don't understand.

Are you saying that the

  SmmRevId < AMD_SMM_MIN_REV_ID_X64

check will always evaluate to FALSE, and so the "return EFI_NOT_FOUND"
branch is dead code?
[Attar, AbdulLateef (Abdul Lateef)] that's right it always evaluate to FALSE
If that's the case, then the patch seems right, but *why* is SmmRevId always greater than or equal to AMD_SMM_MIN_REV_ID_X64?
[Attar, AbdulLateef (Abdul Lateef)] as per AMD64 Programmer's manual 10.2.4 SMM-Revision Identifier.
First 16bit contains the version, which 0x64 for 64bit architecture.
Bit 16 and bit 17 always set 1.
Hence SmmRevId is always equal to AMD_SMM_MIN_REV_ID_X64.

SmmRevId is used to tell apart x86 from x64 (i.e., to distinguish the two formats of the save state map). Is the intent of this patch to remove 32-bit (x86) support?
[Attar, AbdulLateef (Abdul Lateef)]  Nope, we are not removing the x86(32bit) support.
@Lin, Jacque can you modify the logic like below, so that we will honor x86(32bit).

if (MmSaveStateGetRegisterLma () == EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT) {
    //
    // Get SMM Revision ID
    //
    MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);
    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
      return EFI_NOT_FOUND;
    }
}
That makes me uncomfortable, because it could break SMM support in
*IA32* OVMF. Note that commit f2188fe5d155 ("OvmfPkg: Uses MmSaveStateLib library", 2023-07-03) also updated "OvmfPkg/OvmfPkgIa32.dsc".

In fact, I'm worried that the conversion of OVMF to MmSaveStateLib may have been incorrect. Note that commit f2188fe5d155 removed the following comment from "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c":

> //
> // No support for I/O restart
> //

Furthermore, commit f2188fe5d155 removed the following functions:
GetRegisterIndex(), ReadSaveStateRegisterByIndex(), SmmCpuFeaturesReadSaveStateRegister(),
SmmCpuFeaturesWriteSaveStateRegister().

In particular, the latter two functions contained comments and code
like:

>   //
>   // Check for special EFI_SMM_SAVE_STATE_REGISTER_IO
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_IO) {
>     return EFI_NOT_FOUND;
>   }

and

>   //
>   // Writes to EFI_SMM_SAVE_STATE_REGISTER_IO are not supported
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_IO) {
>     return EFI_NOT_FOUND;
>   }

All of these came from Paolo's original commit 4036b4e57cce ("OvmfPkg:
SmmCpuFeaturesLib: implement SMRAM state save map access", 2015-11-30).

Consider the following commits from Paolo (including 4036b4e57cce):

> commit 86d71589c1fdb099c04a0f9e548fe868a2fef266
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:27 2015 +0000
>
>     OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg
>
>     The next patches will customize the implementation, but let's start from
>     the common version to better show the changes.

and

> commit d7e71b2925012c9706d1d044ca466173aac802a8
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:32 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits
>
>     SMRR, MTRR, and SMM Feature Control support is not needed on a virtual
>     platform.

and

> commit 4036b4e57ccef4e0fa48d8389acf6390826c2bed
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:37 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access
>
>     This implementation copies SMRAM state save map access from the
>     PiSmmCpuDxeSmm module.
>
>     The most notable change is:
>
>     - dropping support for EFI_SMM_SAVE_STATE_REGISTER_IO
>
>     - changing the implementation of EFI_SMM_SAVE_STATE_REGISTER_LMA to use
>       the SMM revision id instead of a local variable (which
>       UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c initializes from CPUID's LM
>       bit).  This accounts for QEMU's implementation of x86_64, which always
>       uses revision 0x20064 even if the LM bit is zero.

and

> commit c1fcd80bf42e6b1e91c1c742d222f1ba421b1d1d
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Mon Nov 30 18:46:42 2015 +0000
>
>     OvmfPkg: SmmCpuFeaturesLib: customize state save map format
>
>     This adjusts the previously introduced state save map access functions, to
>     account for QEMU and KVM's 64-bit state save map following the AMD spec
>     rather than the Intel one.

Were these QEMU-specific differences considered when OVMF was migrated to MmSaveStateLib, in commit?

Before the patches for TianoCore Bugzilla 4182 (commit range ad7d3ace1ad4..f2188fe5d155), we used to have the following call tree, for EFI_SMM_CPU_PROTOCOL.ReadSaveState, in OVMF:

  SmmReadSaveState()                      [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
    SmmCpuFeaturesReadSaveStateRegister() [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]
      return EFI_NOT_FOUND
      for EFI_SMM_SAVE_STATE_REGISTER_IO

Thus, the protocol member function would propagate EFI_NOT_FOUND outwards, and ReadSaveStateRegister() [UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c] would not be reached. In particular, the register would not be accessed at all.

After commit f2188fe5d155 however, we have this, in OVMF:

  SmmReadSaveState()          [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
    MmSaveStateReadRegister() [UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c]

Namely:

>   // Check for special EFI_MM_SAVE_STATE_REGISTER_IO
>   if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {
>     //
>     // Get SMM Revision ID
>     //
>     MmSaveStateReadRegisterByIndex (CpuIndex,
> AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId),
> &SmmRevId);
>
>     //
>     // See if the CPU supports the IOMisc register in the save state
>     //
>     if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
>       return EFI_NOT_FOUND;
>     }
>
>     // Check if IO Restart Dword [IO Trap] is valid or not using bit 1.
>     if (!(CpuSaveState->x64.IO_DWord & 0x02u)) {
>       return EFI_NOT_FOUND;
>     }

So, indeed. The patches for TianoCore Bugzilla 4182 were not correct for OVMF.

And *this* patch would make things even worse.

As Paolo's commit 4036b4e57cce above states, QEMU #defines SMM_REVISION_ID as 0x00020064 (see "target/i386/tcg/sysemu/smm_helper.c").

That means that, *even after* commit f2188fe5d155, the check

    if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {
      return EFI_NOT_FOUND;
    }

would prevent OVMF from accessing EFI_MM_SAVE_STATE_REGISTER_IO, because
AMD_SMM_MIN_REV_ID_X64 is #defined as 0x30064 in "MdePkg/Include/Register/Amd/SmramSaveStateMap.h".

So the condition would actually evaluate to TRUE,
MmSaveStateReadRegister() would return EFI_NOT_FOUND, and
SmmReadSaveState() -- the EFI_SMM_CPU_PROTOCOL.ReadSaveState protocol member function -- would also return EFI_NOT_FOUND.

Put differently, that particular check would maintain the consistency of OVMF's behavior regarding EFI_MM_SAVE_STATE_REGISTER_IO, *even though* the patches for TianoCore Bugzilla 4182 (commit range
ad7d3ace1ad4..f2188fe5d155) may have been incorrect in other aspects.

And then the *present* patch would break *that* last resort.

In short, the statement "QEMU/OVMF uses the AMD save state map" is not accurate. In most senses, yes, but not exactly.

Therefore this patch is not acceptable for OVMF (unless you can prove that it makes absolutely no difference for either of IA32, IA32X64, and
X64 OVMF).


... Back to the already upstream patches. Again from Paolo's original commit 4036b4e57cce ("OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access", 2015-11-30), we used to have the following access logic for EFI_SMM_SAVE_STATE_REGISTER_LMA, at commit ad7d3ace1ad4 (i.e., before the conversion), in SmmCpuFeaturesReadSaveStateRegister()
[OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]

>   //
>   // Check for special EFI_SMM_SAVE_STATE_REGISTER_LMA
>   //
>   if (Register == EFI_SMM_SAVE_STATE_REGISTER_LMA) {
>     //
>     // Only byte access is supported for this register
>     //
>     if (Width != 1) {
>       return EFI_INVALID_PARAMETER;
>     }
>
>     CpuSaveState = (QEMU_SMRAM_SAVE_STATE_MAP
> *)gSmst->CpuSaveState[CpuIndex];
>
>     //
>     // Check CPU mode
>     //
>     if ((CpuSaveState->x86.SMMRevId & 0xFFFF) == 0) {
>       *(UINT8 *)Buffer = 32;
>     } else {
>       *(UINT8 *)Buffer = 64;
>     }
>
>     return EFI_SUCCESS;
>   }

But now we have, in MmSaveStateReadRegister()
[UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c]:

>   // Check for special EFI_MM_SAVE_STATE_REGISTER_LMA
>   if (Register == EFI_MM_SAVE_STATE_REGISTER_LMA) {
>     // Only byte access is supported for this register
>     if (Width != 1) {
>       return EFI_INVALID_PARAMETER;
>     }
>
>     *(UINT8 *)Buffer = MmSaveStateGetRegisterLma ();
>
>     return EFI_SUCCESS;
>   }

where MmSaveStateGetRegisterLma()
[UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c] does the following:

>   UINT32  LMAValue;
>
>   LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA;
>   if (LMAValue) {
>     return EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT;
>   }
>
>   return EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT;

I don't know if this is still correct on QEMU or not, but this is not what the original code did. The original code checked SMMRevId, as Paolo said in the message of commit 4036b4e57cce; the new code reads the EFER MSR.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110430): https://edk2.groups.io/g/devel/message/110430
Mute This Topic: https://groups.io/mt/102269908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

[Laszlo Ersek]

On 10/31/23 11:43, Attar, AbdulLateef (Abdul Lateef) via groups.io wrote:
> [Public]
> 
> Hi Laszlo and @Lin, Jacque
>         Please find my response inline.
> Thanks
> AbduL
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, October 30, 2023 7:34 PM
> To: devel@edk2.groups.io; Lin, Jacque <HsienChieh.Lin@amd.com>
> Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Chang, Abner <Abner.Chang@amd.com>; Gerd Hoffmann <kraxel@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib

> Are you saying that the
> 
>   SmmRevId < AMD_SMM_MIN_REV_ID_X64
> 
> check will always evaluate to FALSE, and so the "return EFI_NOT_FOUND"
> branch is dead code?
> [Attar, AbdulLateef (Abdul Lateef)] that's right it always evaluate to FALSE
> If that's the case, then the patch seems right, but *why* is SmmRevId always greater than or equal to AMD_SMM_MIN_REV_ID_X64?
> [Attar, AbdulLateef (Abdul Lateef)] as per AMD64 Programmer's manual 10.2.4 SMM-Revision Identifier.
> First 16bit contains the version, which 0x64 for 64bit architecture.
> Bit 16 and bit 17 always set 1.
> Hence SmmRevId is always equal to AMD_SMM_MIN_REV_ID_X64.

That may apply to physical hardware platforms, but does not apply to
QEMU/KVM, and this library instance is also used in OVMF (on QEMU/KVM),
since commit commit f2188fe5d155 ("OvmfPkg: Uses MmSaveStateLib
library", 2023-07-03).

Therefore, whatever you do in this library instance, must consider the
impact on OVMF on QEMU/KVM.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110432): https://edk2.groups.io/g/devel/message/110432
Mute This Topic: https://groups.io/mt/102269908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-