Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
|
Laszlo Ersek
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275The above explanation is inexact. There are several typos ("APIC" is incorrect, "ACPI" would be correct, for the TimerLib instance in question), but that's really just a side observation. The precise explanation is the following library instance dependency chain: OvmfPkg/AmdSevDxe/AmdSevDxe.inf -----> MemEncryptSevLib class -----> "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" instance -[*]-> VmgExitLib class -----> "OvmfPkg/Library/VmgExitLib/VmgExitLib.inf" instance -----> LocalApicLib class -----> "UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf" instance -----> TimerLib class -----> "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance -----> PciLib class -----> "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" instance -----> PciExpressLib class -----> "MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf" instance The link (or dependency) marked with [*] is introduced in patch #26 ("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table"). That's the change that triggers the symptom. (In combination with you testing on Q35, because on i440fx, the DxePciLibI440FxQ35 lib instance accesses PCI config space via the 0xCF8, 0xCFC IO Ports, and those are unaffected by SEV-ES.) The symptom is somewhat "unjustified", because at the end of the series, the AmdSevDxe driver makes no calls to actual TimerLib APIs (I checked -- I disassembled the "AmdSevDxe.debug" file with "objdump -S", and there is no call to any API declared in the "TimerLib.h" class header). However, the ECAM (MMCONFIG) access is still triggered, because the AcpiTimerLibConstructor() function, in "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c", is the constructor for the "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance, and AcpiTimerLibConstructor() calls PciRead32(). If you check the "OvmfPkg/OvmfPkgX64.dsc" file, you'll find that the PciLib class is resolved to "MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf" by default, and to "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" for the following module types: - DXE_DRIVER, - DXE_RUNTIME_DRIVER, - SMM_CORE, - DXE_SMM_DRIVER, - UEFI_DRIVER, - UEFI_APPLICATION. The consequence is that modules strictly after the DXE_CORE get dynamically enabled extended config space access (ECAM) on Q35 via the PciLib class, whereas all modules strictly before the DXE_CORE, and the DXE_CORE itself, are restricted to normal config space (IO Ports 0xCF8 / 0xCFC) via the PciLib class. AmdSevDxe is a DXE_DRIVER, so it used to get DxePciLibI440FxQ35 as well. The solution should be simple. In the AmdSevDxe driver specifically, we need no access to extended PCI config space. Accessing normal PCI config space, via IO Ports 0xCF8 / 0xCFC, should suffice. That can be achieved with the following module-scope override: diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc( For consistency, all DSC files that include "OvmfPkg/AmdSevDxe/AmdSevDxe.inf" should be modified similarly: - OvmfPkg/AmdSev/AmdSevX64.dsc - OvmfPkg/Bhyve/BhyveX64.dsc - OvmfPkg/OvmfPkgIa32X64.dsc - OvmfPkg/OvmfPkgX64.dsc - OvmfPkg/OvmfXen.dsc ) Therefore, please try dropping this patch, and modifying patch#26 instead -- the above module-scope override (for 5 DSC files) should be squashed into patch#26, *and* the explanation I provided above should be included in the commit message of patch#26. ... Correction: you have an independent bug in the series that affects my above analysis. Namely, you *seem* to add the VmgExitLib dependency to the "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" library instance, in patch#26. That's where you modify the INF file. But that's wrong: in patch#21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system RAM"), you add a VmgInit() call to the same library instance, via the new file "OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c". The bug in that patch is clear from the fact that you introduce an #include <Library/VmgExitLib.h> directive, but that's not mirrored by an appropriate [LibraryClasses] change in the "DxeMemEncryptSevLib.inf" file. (The other two lib instance INF files, "SecMemEncryptSevLib.inf" and "PeiMemEncryptSevLib.inf" *are* modified as needed.) So you even need to move some stuff from patch#26 to patch#21, and *then* squash the above module-scope override (and explanation) into patch#21. A significant amount of work is needed on this series. I'll stop reviewing RFC v2 here, because I don't want to look at the remaining patches deeply as long as code movements etc are going to affect them. Please post the next version -- assuming no other reviewer would like to finish reviewing this version first! Thanks Laszlo
|
|
|