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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

+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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-