Re: [edk2-devel] [PATCH v2 02/31] OvmfPkg: Create platform XenOvmf

["Anthony PERARD" <anthony.perard@citrix.com>]

On Wed, Apr 10, 2019 at 11:32:10AM +0200, Laszlo Ersek wrote:
> On 04/09/19 13:08, Anthony PERARD wrote:
> > This is a copy of OvmfX64, removing VirtIO and some SMM.
> > 
> > This new platform will be changed to make it works on two types of Xen
> > guest: HVM and PVH.
> > 
> > Compare to OvmfX64, this patch:
> > 
> > - changed: PLATFORM_GUID, OUTPUT_DIRECTORY, FLASH_DEFINITION
> > - removed: VirtioLib class resolution
> > - removed: all UEFI_DRIVER modules for virtio devices
> > - removed: DXE_SMM_DRIVER and SMM_CORE lib class resolutions
> > - removed: DXE_SMM_DRIVER and SMM_CORE FDF rules
> > - removed: Everything related to SMM_REQUIRE==true
> > - removed: Everything related to SECURE_BOOT_ENABLE==true
> > - removed: Everything related to TPM2_ENABLE==true
> > - changed: PcdPciDisableBusEnumeration dynamic default flipped to TRUE
> > - changed: default FD_SIZE_IN_KB to 2M.
> > 
>
> (1) in the commit message, it might be worth mentioning (additionally)
> that we undo commit d272449d9e1e ("OvmfPkg: raise DXEFV size to 11 MB",
> 2018-05-29).

Will do.

> (2) Please don't forget to rename & replace XenOvmf -> OvmfXen (in file
> names, the commit message, and the patch body).

Will do.

> > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/XenOvmf.fdf
> > similarity index 85%
> > copy from OvmfPkg/OvmfPkgIa32X64.fdf
> > copy to OvmfPkg/XenOvmf.fdf
> > index 6c40540202..612ffb2e01 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> > +++ b/OvmfPkg/XenOvmf.fdf
> > @@ -197,9 +190,6 @@ [FV.DXEFV]
> >    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> >    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> >    INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> > -!if $(SMM_REQUIRE) == FALSE
> > -  INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> > -!endif
> >  }
> 
> (3) Here you diverge from the general pattern of:
> - SMM_REQUIRE==TRUE --> drop the full block
> - SMM_REQUIRE==FALSE --> drop the condition, keep the block
> 
> For consistency with the pattern, you sould *in theory* only drop the
> condition.
> 
> Let's see what this change effects.
> 
> You remove the driver from the APRIORI DXE file. That means the DXE Core
> will be at liberty to launch the driver at any time, provided DEPEXes
> are satisfied.
> 
> This wouldn't be correct for the original FDF file, because there we
> need an arbitration between this driver and the emulation driver,
> dependent on flash detection. Therefore the original platform first
> launches the flash driver, and falls back to the emu driver if flash is
> not present. Except in case of SMM_REQUIRE, in which case *both* of
> those drivers are replaced by the SMM flash driver (and no arbitration
> occurs, because flash is a hard requirement).
> 
> On Xen, SMM_REQUIRE is indeed assumed FALSE, but we also don't really
> need the arbitration, because we know the flash detection will always
> fail. Therefore, it is correct to remove the flash driver from the
> APRIORI DXE file -- but then, shouldn't we remove the driver itself from
> the DSC and FDF files too?
> 
> I mean, if it doesn't matter *when* it is launched, then you don't need
> to launch it *at all*, I believe.
> 
> If you agree, then it's OK (but not required) to split this change to a
> separate patch -- as I said it diverges from the general pattern and may
> deserve a bit more explanation.

I think that was removed by mistake, otherwise I think I would have
removed QemuFlashFvbServicesRuntimeDxe completely. I do some tests, and
if I do need to remove it from the APRIORI list, I do that in a
separated patch.

> (4) You might want to remove/revert SEV-related modules as well (I'm
> unsure if AMD SEV works on Xen -- if it does, then please ignore):
> 
> - IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> 
> (refer to "git blame" to see what to revert it to)
> 
> -
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
> 
> - NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> 
> - OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> - OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> 
> This can be a separate patch again.

Thanks for the suggestion. It probably doesn't hurt to keep the
SEV-related modules, so I think I'll keep them for now.

> (5) I think you could change the QemuFwCfgS3Lib resolution to
> "BaseQemuFwCfgS3LibNull.inf", and then remove all the S3 modules --
> search OvmfPkg, and generally the edk2 tree, for " PcdAcpiS3Enable".
> 
> See also module names having "S3" in the name, in the FDF/DSC files.
> 
> This would probably be a separate patch.

I'll have a look, thanks.

-- 
Anthony PERARD