Re: [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
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
-----> 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:
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:
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
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
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!