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; Thu, 11 Apr 2019 00:34:06 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E2B83086218; Thu, 11 Apr 2019 07:34:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 69DC2600D4; Thu, 11 Apr 2019 07:34:03 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 02/31] OvmfPkg: Create platform XenOvmf To: Jordan Justen , Anthony PERARD , devel@edk2.groups.io Cc: Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-3-anthony.perard@citrix.com> <155488970861.20447.12525571872411959890@jljusten-skl> <155492143006.23894.8253130602441997952@jljusten-skl> From: "Laszlo Ersek" Message-ID: Date: Thu, 11 Apr 2019 09:34:02 +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: <155492143006.23894.8253130602441997952@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Thu, 11 Apr 2019 07:34:05 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/10/19 20:37, Jordan Justen wrote: > On 2019-04-10 07:27:15, Laszlo Ersek wrote: >> On 04/10/19 11:48, Jordan Justen wrote: >>> On 2019-04-09 04:08:15, 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. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Anthony PERARD >>>> --- >>>> OvmfPkg/{OvmfPkgX64.dsc => XenOvmf.dsc} | 202 +------------------- >>> >>> I guess we all want our name to be first :), but OvmfXen seems more >>> like the pattern that edk2 uses when making sub-platforms. > > For example: ArmVirtPkg/ArmVirtXen.dsc Right, we all agree on this. > >>> >>> Also, in some cases we've used !ifdef type things to keep dsc and fdf >>> files common. Would that not be workable here? Maybe it would get too >>> ugly. :\ >> >> I've been happy with a similar Xen<->QEMU split under ArmVirtPkg. >> Duplications in updates are usually not a huge burden, and keeping the >> files separate has proved very helpful for determining >> maintainer/reviewer/tester responsibility. >> >>> It could help to prevent having to sync dsc changes across, and >>> prevent changes from being omitted for Xen on accident. >> >> True, but in my experience that's been the smaller problem. The larger >> problem has been modifying Xen stuff in unintended ways (= regressing >> Xen), because we can't test (or don't even notice) the Xen-side >> implications of changes made to common source. > > Does that mean you avoid changing ArmVirtXen.dsc entirely, or you try > to update it when it seems likely to not cause trouble? I could see > unintended breakage either way. Under ArmVirtPkg, we have three platforms: ArmVirtQemu, ArmVirtQemuKernel, and ArmVirtXen. All of these have separate DSC files. All include ArmVirt.dsc.inc. In addition, all three have FDF files, but ArmVirtQemu and ArmVirtQemuKernel share (include) ArmVirtQemuFvMain.fdf.inc. ArmVirtQemu and ArmVirtQemuKernel differ in that the latter is supposed to be launched with "-kernel" on QEMU, not via pflash. I primarily care about ArmVirtQemu. I always start modifications there. Then I evaluate whether the change makes sense for ArmVirtQemuKernel, and rely on Ard to confirm that. ArmVirtXen is the last thought for me, and I try to make a "best effort" to figure out the impact. When I'm stongly in doubt re: Xen, I avoid modifying ArmVirtXen. When I sort of suspect that modifying Xen is valid/necessary, then I (a) either modify ArmVirtXen explicitly, or (b) modify ArmVirt.dsc.inc *AND* talk about Xen explicitly in the commit message, and/or the cover letter. Case (b) is quite rare. When it happens, it usually follows immediately (a) -- I might change ArmVirtXen, and then realize something is now shared between Xen and QEMU. It's then a separate decision whether I actually want to merge that thing into ArmVirt.dsc.inc -- sometimes that's not the best decision (exactly because it increases shared review responsibility for the future). In both case (a) and (b), I CC the Xen reviewers. The point is that "Xen" is mentioned somewhere explicitly (diffstat or commit message). This lets us decide more clearly whether Xen reviewers should participate -- when they don't have to, that saves us all time, and so when they *do* need to, their responsibility is both big and clear. If, for QEMU feature development or bugfixing, we kept modifying files that were by default processed in Xen builds too, then Xen people would be bombarded with review requests they'd rather ignore. Their attention would wane and they'd miss (or greatly delay) the reviews when they should indeed follow up (and hopefully quickly). It's not a 100% "scientific" workflow, and you are right that both approaches can cause issues by omission. In my experience the split tends to be safer (and easier on human resources), in practice. > Anyway, it sounds like it's generally working out okay with > ArmVirtXen, so it seems okay to follow that under OvmfPkg. Thanks! Laszlo > > -Jordan >