UefiCpuPkg: Remove AMD 32-bit SMRAM save state map

[philnoh2]

Per AMD64 Architecture Programmer’s Manual Volume 2: System Programming - 10.2.3 SMRAM State-Save Area (Rev 24593), the AMD64 architecture does not use the legacy SMM state-save area format (Table 10-2) for 32-bit SMRAM save state map. Clean up codes for the invalid save state map.

Description

It is based on the AMD manual information (Table 10-2) below.

https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf#391

• Breaking change?
  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.
• Impacts security?
  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• Includes tests?
  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

With this update, we have fixed an issue caused by referencing the invalid save state map on AMD systems based on edk2-stable-202405 core (or above).

Integration Instructions

N/A

[abdattar]

Code changes looks good and Approved.
Need approval from Ovmf maintainers to push the changes.

[lgao4]

The change in MdePkg is good to me.

[abdattar]

Hi @kraxel, @jyao1,
Gentle reminder, please review the Ovmf patch.
Thanks
AbduL

[kraxel]

Even if physical amd processors don't use the legacy format -- the linux kernel (kvm specifically) uses it in case the processor is in 32-bit mode. So I'd expect this change breaks OvmfPkg/OvmfPkgIa32.dsc with -D SMM_REQUIRE=TRUE.

[philnoh2]

Even if physical amd processors don't use the legacy format -- the linux kernel (kvm specifically) uses it in case the processor is in 32-bit mode. So I'd expect this change breaks OvmfPkg/OvmfPkgIa32.dsc with -D SMM_REQUIRE=TRUE.

Ater reviewing the feedback internally, I have excluded the changes in MdePkg and OvmfPkg to support compatibility with the existing Ovmf environment like the case. In the result, the updated PR only includes the changes for AMD based files in UefiCpuPkg. Thank you.

[kraxel]

Ater reviewing the feedback internally, I have excluded the changes in MdePkg and OvmfPkg to support compatibility with the existing Ovmf environment like the case. In the result, the updated PR only includes the changes for AMD based files in UefiCpuPkg. Thank you.

Well, OvmfPkg/OvmfPkgIa32.dsc uses UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf, so I suspect that will still break 32-bit OVMF ...

[abdattar]

I can see that CI has been passed for 32bit OVMF.
PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (Platform_CI OVMF_IA32X64_DEBUG)
PlatformCI_OvmfPkg_Windows_VS2019_PR (Platform_CI OVMF_IA32_DEBUG)

[kraxel]

Checked again. The format used by kvm does not depend on the current processor mode, but on the processor capabilities (probably matching physical hardware). In case the (virtual) processor does not support long mode (qemu -cpu host,lm=off) the legacy format will be used.

[kraxel]

The OVMF smbase relocation code does not take the MmSaveState{Read,Write}Register() indirection but accesses the struct fields directly. So master branch continues to work fine with both formats even without MmSaveStateLib support.

[philnoh2]

Ater reviewing the feedback internally, I have excluded the changes in MdePkg and OvmfPkg to support compatibility with the existing Ovmf environment like the case. In the result, the updated PR only includes the changes for AMD based files in UefiCpuPkg. Thank you.

Well, OvmfPkg/OvmfPkgIa32.dsc uses UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf, so I suspect that will still break 32-bit OVMF ...

I understand the point. Let me add some comments. Like the description, it is a required update for AMD processors. It updates the following 3 libraries in AmdCpuPkg.

UefiCpuPkg :
UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
UefiCpuPkg/Library/SmmRelocationLib/AmdSmmRelocationLib.inf

I can check that currently OvmfPkg (e.g. OvmfPkgIa32.dsc) has the following usage related to them.

OvmfPkg :
UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
OvmfPkg/Library/SmmRelocationLib/SmmRelocationLib.inf

Therefore, I found that AmdMmSaveStateLib.inf only came from UefiCpuPkg. Like other 2 libraries, I think that it would be better for OvmfPkg to contain the library together to prevent a possible gap according to some updates of UefiCpuPkg like the update.

I would like OvmfPkg team to review this suggestion - locating the previous version of AmdMmSaveStateLib.inf to OvmfPkg and use it if needed. As a general thought, I think that the emulation side may be reviewed to sync with physical AMD processors.

[kraxel]

See my last two comments (not sure you have noticed them while writing your long answer). KVM behavior should be pretty much in sync with physical AMD processors. Only vcpus without long mode support use the legacy format.

On physical hardware the legacy format simply doesn't matter any more.

For virtual hardware it is a bit different because it is easy to turn on and off all kinds of features (including long mode) for the virtual cpus. Big question is: How much do we care? Running 32-bit EFI already is a very niche thing. Running that on a host without long mode support (or long mode disabled in the guest configuration) is even more rare.

I'd say removing legacy format support from OVMF is acceptable. We should add a check + log an error though, so in case someone traps into that rare corner case it is at least obvious what the problem is.

With the check in place we can apply the other two patches. We can probably also drop OvmfPkg/Library/SmmRelocationLib/SmmRelocationLib.inf and use UefiCpuPkg/Library/SmmRelocationLib/SmmRelocationLib.inf instead. I think there also is some code in OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf which is dead since the switch to SmmRelocationLib.

[philnoh2]

See my last two comments (not sure you have noticed them while writing your long answer). KVM behavior should be pretty much in sync with physical AMD processors. Only vcpus without long mode support use the legacy format.

On physical hardware the legacy format simply doesn't matter any more.

For virtual hardware it is a bit different because it is easy to turn on and off all kinds of features (including long mode) for the virtual cpus. Big question is: How much do we care? Running 32-bit EFI already is a very niche thing. Running that on a host without long mode support (or long mode disabled in the guest configuration) is even more rare.

I'd say removing legacy format support from OVMF is acceptable. We should add a check + log an error though, so in case someone traps into that rare corner case it is at least obvious what the problem is.

With the check in place we can apply the other two patches. We can probably also drop OvmfPkg/Library/SmmRelocationLib/SmmRelocationLib.inf and use UefiCpuPkg/Library/SmmRelocationLib/SmmRelocationLib.inf instead. I think there also is some code in OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf which is dead since the switch to SmmRelocationLib.

I understand. Including the 2 comments, I think that the information is helpful to understand OvmfPkg status. For the update points of OvmfPkg on the information, let me leave them to OvmfPkg maintainers for better support. Regarding this PR's review, I have sent you an email. Let me expect that the email is used to discuss an issue or question with us. Thank you for your review.