From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 162.221.156.55, mailfrom: prvs=0011d4d25=anthony.perard@citrix.com) Received: from SMTP03.CITRIX.COM (SMTP03.CITRIX.COM [162.221.156.55]) by groups.io with SMTP; Mon, 15 Apr 2019 03:53:48 -0700 X-IronPort-AV: E=Sophos;i="5.60,353,1549929600"; d="scan'208";a="83565253" Date: Mon, 15 Apr 2019 11:53:44 +0100 From: "Anthony PERARD" To: Laszlo Ersek CC: , Jordan Justen , "Ard Biesheuvel" , Julien Grall , Subject: Re: [edk2-devel] [PATCH v2 02/31] OvmfPkg: Create platform XenOvmf Message-ID: <20190415105344.GA1354@perard.uk.xensource.com> References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-3-anthony.perard@citrix.com> <40c8064b-a161-1cfd-c3b1-7495c8115b74@redhat.com> MIME-Version: 1.0 In-Reply-To: <40c8064b-a161-1cfd-c3b1-7495c8115b74@redhat.com> User-Agent: Mutt/1.11.4 (2019-03-13) Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline 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