From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 09 Apr 2019 01:09:31 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 50BD430050B5; Tue, 9 Apr 2019 08:09:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-114.rdu2.redhat.com [10.10.120.114]) by smtp.corp.redhat.com (Postfix) with ESMTP id 34A3E5D71B; Tue, 9 Apr 2019 08:09:28 +0000 (UTC) Subject: Re: [PATCH v2 00/31] Specific platform to run OVMF in Xen PVH and HVM guests From: "Laszlo Ersek" To: Anthony PERARD Cc: Julien Grall , Ard Biesheuvel , Jordan Justen , xen-devel@lists.xenproject.org, Leif Lindholm , edk2-devel-groups-io References: <20190408142408.30419-1-anthony.perard@citrix.com> <6c00d5f5-e187-5fe2-8b3e-301ec51efb6b@redhat.com> Message-ID: <34ead1d8-12d8-91b5-9767-3d706121a6b0@redhat.com> Date: Tue, 9 Apr 2019 10:09:28 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <6c00d5f5-e187-5fe2-8b3e-301ec51efb6b@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Tue, 09 Apr 2019 08:09:31 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/08/19 17:50, Laszlo Ersek wrote: > On 04/08/19 16:23, Anthony PERARD wrote: >> Patch series available in this git branch: >> https://xenbits.xen.org/git-http/people/aperard/ovmf.git br.platform-xen-pvh-v2 >> >> Hi, >> >> I've started to create a Xen specific platform, in OvmfPkg/XenOvmf.dsc >> with the goal to make it work on both Xen HVM and Xen PVH. > > The previous version of this feature dates back to Dec 2016 / Jan 2017; > I haven't forgotten about it, and I'm happy that you are adding the > feature as a separate platform DSC, as I requested. > >> The first few patches only create the platform and duplicate some code >> from OvmfPkg and the later patches makes OVMF boot in a Xen PVH guest >> and can boot a Linux guest. >> >> After this patch series, I'd like to wait a bit before removing Xen >> support from the OvmfPkg*.dsc, to allow time to switch to the new Xen >> only platform, maybe 1 year. > > I welcome this proposal very much. This will eliminate a significant > amount of complexity in the current modules, allow for > easier-to-understand maintenance assignments/responsibilities, and pave > the way for features that are unique to either Xen or QEMU (in > particular in the variable storage area, for which Xen has just received > a dedicated userspace daemon IIRC, and for which -- on QEMU -- I had > attempted to make pflash a conditional hard requirement, in the past, > but failed due to OvmfPkg*.dsc targeting Xen too). > >> Question: >> >> Should we start moving these to a different *Pkg? Like it's done for >> ArmPkg and ArmVirtPkg? Maybe XenPkg. > > I'm pretty happy with the current package structure. ArmPkg is for both > physical and virtual hardware. ArmVirtPkg is virt-only, and we already > have separate platform DSCs/FDFs between Xen (ArmVirtXen) and QEMU/KVM > (ArmVirtQemu[Kernel]). Xen- and QEMU/KVM-specific drivers and library > instances peacefully coexist under ArmVirtPkg; the DSCs/FDFs include > them as appropriate. > > The same should map nicely to OvmfPkg. x86 stuff that targets both > physical and virtual hardware belongs under PcAtChipsetPkg and > UefiCpuPkg. Virt-only stuff should go under OvmfPkg; Xen-specific and > QEMU/KVM-specific modules can coexist under OvmfPkg. It's sufficient if > the platform DSCs cherry-pick them as appropriate. > > And, if there are Xen-specific (but not arch-specific) PEIMs / drivers / > libraries that work alike on ARM and x86, they should go under OvmfPkg, > as ArmVirtPkg can (and already does) pull Xen-only modules from OvmfPkg. > See for example, in ArmVirtXen.dsc: > > SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf > XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf > > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > >> To build and boot: >> >> To build, simply run OvmfPkg/build.sh -p OvmfPkg/XenOvmf.dsc > > (1) To stick with the ArmVirt pattern, we should initially call this > platform OvmfXen. > > (2) And, once you remove the Xen bits from the current > OvmfPkg*.dsc files, we should likely rename those to OvmfQemu*dsc. > > I'll try to process this series in a timely manner. My main focus will > be possible regressions on QEMU/KVM, and formalities (license blocks, > signoffs, edk2 coding style, build issues that I might notice in > review). > > I can't provide testing feedback, but xen-devel subscribers (and any > other Xen users too, obviously) are super welcome to test & report > results! > > (3) Please file a BZ at about this > feature, and assign it to yourself. The BZ should keep track of all > versions of the patch series (from the mailing list archive). > > (4) Ideally, the release notes for the edk2 stable release in which the > feature will appear, should mention the feature (via linking the BZ): > > > (5) The BZ should be referenced in all the commit messages. > > (6) The new edk2 development mailing list is at: > > https://edk2.groups.io/g/devel > > Please subscribe there, and resend this series to that address, i.e. > . I'm CC'ing the new address myself, for this > initial response, but I'd prefer the rest of my comments to go only to > the new list (without manually updating the CC list on every response). (7) I forgot to mention that, due to being very close to being fixed / pushed, the the "Contributed-under" lines near your signoffs will no longer be necessary. Thanks Laszlo > > Thanks! > Laszlo > >> Then use OVMF.fd as a kernel of a pvh guest config file (with >> xl/libxl). >> >> Patch series available in this git branch: >> https://xenbits.xen.org/git-http/people/aperard/ovmf.git br.platform-xen-pvh-v2 >> >> Anthony PERARD (31): >> OvmfPkg/ResetSystemLib: Add missing dependency on PciLib >> OvmfPkg: Create platform XenOvmf >> OvmfPkg: Introduce XenResetVector >> OvmfPkg: Introduce XenPlatformPei >> OvmfPkg/XenOvmf: Creating an ELF header >> OvmfPkg/XenResetVector: Add new entry point for Xen PVH >> OvmfPkg/XenResetVector: Saving start of day pointer for PVH guests >> OvmfPkg/XenResetVector: Allow to jumpstart from either hvmloader or >> PVH >> OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU >> OvmfPkg/XenPlatformPei: Detect OVMF_INFO from hvmloader >> OvmfPkg/XenPlatformPei: Use mXenHvmloaderInfo to get E820 >> OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct >> OvmfPkg/Library/XenPlatformLib: New library >> OvmfPkg/AcpiPlatformDxe: Use PVH RSDP if exist >> OvmfPkg/XenHypercallLib: Enable it in PEIM >> OvmfPkg/XenPlatformPei: Introduce XenHvmloaderDetected >> OvmfPkg/XenPlatformPei: Reserve hvmloader's memory only when it as >> runned >> OvmfPkg/XenPlatformPei: Setup HyperPages earlier >> OvmfPkg/XenPlatformPei: Introduce XenPvhDetected >> OvmfPkg: Import XENMEM_memory_map hypercall to Xen/memory.h >> OvmfPkg/XenPlatformPei: Get E820 table via hypercall ... >> OvmfPkg/XenPlatformPei: Rework memory detection >> OvmfPkg/XenPlatformPei: Reserve VGA memory region, to boot Linux >> OvmfPkg/XenPlatformPei: Ignore missing PCI Host Bridge on Xen PVH >> OvmfPkg/PlatformBootManagerLib: Handle the absence of PCI bus on Xen >> PVH >> OvmfPkg/XenOvmf: Override PcdFSBClock to Xen vLAPIC timer frequency >> OvmfPkg/XenOvmf: Introduce XenTimerDxe >> OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn >> OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables >> OvmfPkg: Move XenRealTimeClockLib from ArmVirtPkg >> OvmfPkg/XenOvmf: use RealTimeClockRuntimeDxe from EmbeddedPkg >> >> OvmfPkg/OvmfPkg.dec | 4 + >> ArmVirtPkg/ArmVirtXen.dsc | 2 +- >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> OvmfPkg/{OvmfPkgX64.dsc => XenOvmf.dsc} | 236 ++------ >> OvmfPkg/XenOvmf.fdf | 561 ++++++++++++++++++++ >> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +- >> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 + >> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf | 1 + >> OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf | 2 +- >> ArmVirtPkg/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf => OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf | 30 +- >> {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf | 0 >> OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 35 ++ >> OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 107 ++++ >> OvmfPkg/XenResetVector/XenResetVector.inf | 46 ++ >> OvmfPkg/XenTimerDxe/XenTimerDxe.inf | 49 ++ >> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 6 +- >> OvmfPkg/Include/Guid/XenInfo.h | 8 +- >> OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h | 159 ++++++ >> OvmfPkg/Include/IndustryStandard/Xen/memory.h | 23 + >> OvmfPkg/Include/Library/XenHypercallLib.h | 12 + >> OvmfPkg/Include/Library/XenPlatformLib.h | 59 ++ >> OvmfPkg/Include/OvmfPlatforms.h | 6 + >> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 + >> OvmfPkg/XenPlatformPei/Cmos.h | 58 ++ >> OvmfPkg/XenPlatformPei/Platform.h | 135 +++++ >> OvmfPkg/{PlatformPei => XenPlatformPei}/Xen.h | 0 >> OvmfPkg/XenTimerDxe/XenTimerDxe.h | 183 +++++++ >> OvmfPkg/AcpiPlatformDxe/Xen.c | 41 +- >> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 15 +- >> OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c | 59 ++ >> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c | 3 +- >> OvmfPkg/Library/XenHypercallLib/X86XenHypercall.c | 11 + >> OvmfPkg/Library/XenPlatformLib/XenPlatformLib.c | 75 +++ >> {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.c | 0 >> OvmfPkg/PlatformPei/Xen.c | 3 - >> OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 38 ++ >> OvmfPkg/XenPlatformPei/AmdSev.c | 70 +++ >> OvmfPkg/XenPlatformPei/ClearCache.c | 118 ++++ >> OvmfPkg/XenPlatformPei/Cmos.c | 66 +++ >> OvmfPkg/XenPlatformPei/Fv.c | 82 +++ >> OvmfPkg/XenPlatformPei/MemDetect.c | 498 +++++++++++++++++ >> OvmfPkg/XenPlatformPei/Platform.c | 458 ++++++++++++++++ >> OvmfPkg/XenPlatformPei/Xen.c | 381 +++++++++++++ >> OvmfPkg/XenTimerDxe/XenTimerDxe.c | 361 +++++++++++++ >> generate_elf_header.c | 78 +++ >> OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm | 144 +++++ >> OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm | 87 +++ >> OvmfPkg/XenResetVector/Ia32/Flat32ToFlat64.asm | 66 +++ >> OvmfPkg/XenResetVector/Ia32/PageTables64.asm | 156 ++++++ >> OvmfPkg/XenResetVector/Ia32/SearchForBfvBase.asm | 93 ++++ >> OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm | 75 +++ >> OvmfPkg/XenResetVector/XenResetVector.nasmb | 78 +++ >> 54 files changed, 4526 insertions(+), 262 deletions(-) >> copy OvmfPkg/{OvmfPkgX64.dsc => XenOvmf.dsc} (78%) >> create mode 100644 OvmfPkg/XenOvmf.fdf >> copy ArmVirtPkg/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf => OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf (50%) >> rename {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf (100%) >> create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf >> create mode 100644 OvmfPkg/XenPlatformPei/XenPlatformPei.inf >> create mode 100644 OvmfPkg/XenResetVector/XenResetVector.inf >> create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.inf >> create mode 100644 OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h >> create mode 100644 OvmfPkg/Include/Library/XenPlatformLib.h >> create mode 100644 OvmfPkg/XenPlatformPei/Cmos.h >> create mode 100644 OvmfPkg/XenPlatformPei/Platform.h >> copy OvmfPkg/{PlatformPei => XenPlatformPei}/Xen.h (100%) >> create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.h >> create mode 100644 OvmfPkg/Library/XenPlatformLib/XenPlatformLib.c >> rename {ArmVirtPkg => OvmfPkg}/Library/XenRealTimeClockLib/XenRealTimeClockLib.c (100%) >> create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c >> create mode 100644 OvmfPkg/XenPlatformPei/AmdSev.c >> create mode 100644 OvmfPkg/XenPlatformPei/ClearCache.c >> create mode 100644 OvmfPkg/XenPlatformPei/Cmos.c >> create mode 100644 OvmfPkg/XenPlatformPei/Fv.c >> create mode 100644 OvmfPkg/XenPlatformPei/MemDetect.c >> create mode 100644 OvmfPkg/XenPlatformPei/Platform.c >> create mode 100644 OvmfPkg/XenPlatformPei/Xen.c >> create mode 100644 OvmfPkg/XenTimerDxe/XenTimerDxe.c >> create mode 100644 generate_elf_header.c >> create mode 100644 OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm >> create mode 100644 OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm >> create mode 100644 OvmfPkg/XenResetVector/Ia32/Flat32ToFlat64.asm >> create mode 100644 OvmfPkg/XenResetVector/Ia32/PageTables64.asm >> create mode 100644 OvmfPkg/XenResetVector/Ia32/SearchForBfvBase.asm >> create mode 100644 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm >> create mode 100644 OvmfPkg/XenResetVector/XenResetVector.nasmb >> >