Re: [edk2-devel] APRIORI in RISC-V or Where did OVMF APRIORIs come from?

["Laszlo Ersek" <lersek@redhat.com>]

On 05/07/20 15:53, Ard Biesheuvel wrote:
> On 5/7/20 3:43 PM, Daniel Schaefer wrote:

>> Should I/we try to remove the APRIORI entries from OVMF in a similar
>> way?
>>
>
> Let's get to the bottom of this first. Laszlo may remember why exactly
> those entries are there in the first place, and I suspect it is a
> different issue.
>

Some of the APRIORI entries are very old and not well documented (via
git-blame / git log). I've done some cursory tests now.

(1) "MdeModulePkg/Universal/PCD/Pei/Pcd.inf" in APRIORI PEI goes back to
commit 49ba9447c92d ("Add initial version of Open Virtual Machine
Firmware (OVMF) platform.", 2009-05-27).

I've tried removing it now (eliminating the whole APRIORI PEI file), and
there seem to be no ill effects.

Note that this module is built like this in the DSC file:

  MdeModulePkg/Universal/PCD/Pei/Pcd.inf  {
    <LibraryClasses>
      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
  }

That's because the PCD PEI depends on a number lib classes, and some of
the lib instances used to resolve those lib classes depend on PcdLib.
The "usual" PEI instance of PcdLib would depend on the PCD PPI, hence
the PCD PEI would depend on itself. The above lib instance override
breaks up the cycle.


(2) The same cannot be said about
"MdeModulePkg/Universal/PCD/Dxe/Pcd.inf" in APRIORI DXE. While the
addition of this entry goes back to the same historical commit
49ba9447c92d ("Add initial version of Open Virtual Machine Firmware
(OVMF) platform.", 2009-05-27); it does have a live dependency.

(2a) Namely,
"OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf"
depends on dynamic PCDs, and FvbServicesRuntimeDxe really does have to
be on the APRIORI DXE list. Refer to commit 182eb4562731 ("OvmfPkg: Add
QemuFlashFvbServicesRuntimeDxe to firmware image", 2013-11-12).

In short, FvbServicesRuntimeDxe and EmuVariableFvbRuntimeDxe are
alternatives (dependent on whether QEMU runs with or without pflash),
and dynamic PCDs are used to arbitrate between them.
FvbServicesRuntimeDxe performs the flash detection so it must come
first.

(2b) Now, if OVMF is built with SMM_REQUIRE, then both
FvbServicesRuntimeDxe and EmuVariableFvbRuntimeDxe fall away (pflash is
a hard requirement, and an *SMM* flash driver is used, namely
"OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf"). Per commit
46df0216b0ed ("OvmfPkg: pull in SMM-based variable driver stack",
2015-11-30), the FvbServicesRuntimeDxe driver is then removed from the
APRIORI DXE file as well. This suggests that the PCD DXE driver *too*
could be removed (with nothing depending on it in the APRIORI DXE list),
when SMM_REQUIRE is TRUE. I've tested conditionalizing the PCD DXE
driver like that, and it seems to work.


(3) The "MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf" driver
in the APRIORI DXE file is justified. It goes back to commit
5c3481b0b611 ("OvmfPkg: Use the new DevicePathLib for all platforms",
2013-08-19), and commit 863986b3c8e6.

(3a) This APRIORI DXE entry is needed because of the following
dependency chain:

  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
  ->
  MdePkg/Library/DxeHobLib/DxeHobLib.inf
  ->
  MdePkg/Library/UefiLib/UefiLib.inf
  ->
  MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf

Due to the PCD DXE driver being on the APRIORI DXE list (see point (2)
above), DevicePathDxe has to be as well.

This chain could be probably broken by modifying the DSC file, to link
the PCD DXE driver with the "thick" DevicePathLib instance:
"MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf".

(3b) Building upon (2b), we get tempted to take the PCD DXE driver and
the DevicePathDxe driver *together* off the APRIORI DXE list, if if
SMM_REQUIRE is defined.

Unfortunately, this doesn't work, as AmdSevDxe is also in the APRIORI
DXE file (see the next point), and it comes with the following
dependency chain:

  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
  ->
  MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
  ->
  MdePkg/Library/UefiLib/UefiLib.inf
  ->
  MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf


(4) AmdSevDxe belongs in the APRIORI DXE file. The explanation is given
in commit 24e4ad75546b ("OvmfPkg: Add AmdSevDxe driver", 2017-07-10):

    The driver is being added to the APRIORI DXE file so that, we clear the
    C-bit from MMIO regions before any driver accesses it.

This is not a perfect solution by far. But a better solution would have
required new infrastructure -- see
<https://bugzilla.tianocore.org/show_bug.cgi?id=623> --, and we could
never reach an agreement on that! (Just read through the ticket --
multiple approaches were proposed and some even prototyped (with patches
posted); nothing was accepted.)


Summarizing the status for APRIORI DXE, AmdSevDxe must be on the APRIORI
DXE list, and AmdSevDxe requires DevicePathDxe. "Dxe/Pcd.inf" could be
conditionalized on SMM_REQUIRE=FALSE.

Summarizing the status for APRIORI PEI, I think it could be eliminated
at this point.

Thanks,
Laszlo