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; Wed, 12 Jun 2019 04:52:21 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 73CC0882EF; Wed, 12 Jun 2019 11:52:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-191.ams2.redhat.com [10.36.117.191]) by smtp.corp.redhat.com (Postfix) with ESMTP id B700560BF1; Wed, 12 Jun 2019 11:52:18 +0000 (UTC) Subject: Re: [PATCH v2 0/6] Ovmf: Drop IntelFramework[Module]Pkg dependency To: David Woodhouse , "Wu, Hao A" , "Ni, Ray" , "Justen, Jordan L" , "devel@edk2.groups.io" Cc: Ard Biesheuvel , "Phillips, D Scott" References: <20190611014313.12160-1-hao.a.wu@intel.com> <156023851748.468.9950138266923344967@jljusten-skl> <453b7e3c48014bd651717ccf9d9380356530bc82.camel@infradead.org> <734D49CCEBEEF84792F5B80ED585239D5C1B3908@SHSMSX104.ccr.corp.intel.com> <862b419a-0715-7430-1f68-bbf54143f7b7@redhat.com> <038f0a993eba5d25eb5e499fa4be95fa6717ad3e.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <4bc1c867-31e8-53cc-429c-c68b2085f34a@redhat.com> Date: Wed, 12 Jun 2019 13:52:17 +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: <038f0a993eba5d25eb5e499fa4be95fa6717ad3e.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 12 Jun 2019 11:52:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/12/19 10:03, David Woodhouse wrote: > On Wed, 2019-06-12 at 09:58 +0200, Laszlo Ersek wrote: >> Ray, >> >> On 06/12/19 04:13, Wu, Hao A wrote: >>>> -----Original Message----- >>>> From: Ni, Ray >>>> Sent: Wednesday, June 12, 2019 10:04 AM >>>> To: Wu, Hao A; David Woodhouse; Justen, Jordan L; >>>> devel@edk2.groups.io >>>> Cc: Laszlo Ersek; Ard Biesheuvel; Phillips, D Scott >>>> Subject: RE: [PATCH v2 0/6] Ovmf: Drop IntelFramework[Module]Pkg >>>> dependency >>>> >>>> Hao, >>>> Will the CSM duplication cause any code change that may impact CSM >>>> functionality? >>> >>> Hello Ray, >>> >>> I think there should be no functional impact for the duplication. >>> There is no change to the .C/.H files. >>> >>> Best Regards, >>> Hao Wu >>> >>>> If no, how about firstly duplicate them first? >>>> >>>> David, >>>> Will this approach work for you? >> >> It will not work for me. >> >> Here's the problem: >> >> - I'm not comfortable approving the duplication (or move) under >> OvmfPkg, until David ACKs the patch -- the first patch in the series >> -- that spells out his reviewership for the CSM modules, > > I'll certainly ack that. > >> - I believe David is not comfortable ACKing that patch until he can >> get the CSM build to work again. > > I'm OK with it as long as the submitter has done their own testing and > confirms that it works at least as well as it did before. Is that testing burden really on Hao though? You write "it works at least as well as it did before", and I generally agree with that -- but I doubt that Hao has *ever* built the SeaBIOS CSM (in itself first, and then into the OVMF binary second). IOW I consider Hao doing OvmfPkg users a favor by submitting these code movement patch sets. When some modules become unmaintained / orphaned and are about to be deleted, the minimum we can expect from the earlier module maintainers is to cleanly remove all in-tree platform references. But code movement under OvmfPkg is more than that minimum; it's a courtesy. I don't think we should expect Hao to do this before-after testing. > It does get quite a long way into 16-bit code and then does somewhere > in the CSM itself, after a few legacy BIOS calls. FWIW, I'm getting the following results, *without* Hao's patches: (1) QEMU is built at commit 47fbad45d47a ("Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging", 2019-06-04) (2a) SeaBIOS is built at commit 85137fb5f2df ("virtio-pci: Use %pP format in dprintf() calls", 2019-05-23) (2b) The following out-of-tree patch is applied to SeaBIOS on top of (2a): > diff --git a/src/hw/serialio.c b/src/hw/serialio.c > index 31633443780a..ed918ade9306 100644 > --- a/src/hw/serialio.c > +++ b/src/hw/serialio.c > @@ -116,7 +116,7 @@ qemu_debug_preinit(void) > void > qemu_debug_putc(char c) > { > - if (!CONFIG_DEBUG_IO || !runningOnQEMU()) > + if (!CONFIG_DEBUG_IO) > return; > u16 port = GET_GLOBAL(DebugOutputPort); > if (port) This is because runningOnQEMU() evaluates to false if SeaBIOS is built as CSM (IIRC), yet I'd like to get debug output. (2c) SeaBIOS is built *twice*, once for the CSM, and another time for the Cirrus VGABIOS. Parts of my build script are: > make distclean > cat >.config <<-EOT > CONFIG_CSM=y > CONFIG_QEMU_HARDWARE=y > CONFIG_PERMIT_UNALIGNED_PCIROM=y > CONFIG_DEBUG_LEVEL=20 > CONFIG_ROM_SIZE=192 > EOT > yes "" | make oldconfig > make -j2 out/bios.bin The "bios.bin" file built by this step is copied into the edk2 tree as "OvmfPkg/Csm/Csm16/Csm16.bin". > make distclean > cat >.config <<-EOT > CONFIG_QEMU=y > CONFIG_BUILD_VGABIOS=y > CONFIG_VGA_CIRRUS=y > CONFIG_VGA_PCI=y > CONFIG_DEBUG_LEVEL=20 > EOT > yes "" | make oldconfig > make -j2 out/vgabios.bin The "vgabios.bin" file built by this step is copied to a different directory under the name "vgabios-cirrus.csm.bin". (3a) OVMF is built at edk2 commit e5b4d825afc4 ("MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads", 2019-06-11). (3b) The following out-of-tree patch is applied on top of (3a): > diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c > index 211750c0123b..119b5c9b6fe8 100644 > --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c > +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c > @@ -1231,8 +1231,8 @@ GenericLegacyBoot ( > // > Private->LegacyRegion->Lock ( > Private->LegacyRegion, > - 0xc0000, > - 0x40000, > + 0xe0000, > + 0x10000, > &Granularity > ); > (3c) "OvmfPkg/Csm/Csm16/Csm16.bin" comes from step (2c). (3d) The OVMF build command line is: > build \ > -a X64 \ > -b NOOPT \ > -p OvmfPkg/OvmfPkgX64.dsc \ > -t GCC48 \ > --cmd-len=65536 \ > -D CSM_ENABLE \ > -D FD_SIZE_2MB (4) The QEMU command line is: > qemu-system-x86_64 \ > -nodefaults \ > -m 1024 \ > -M pc,accel=kvm \ > -device cirrus-vga,romfile=vgabios-cirrus.csm.bin \ > -drive if=pflash,readonly,format=raw,file=OVMF_CODE.fd \ > -drive if=pflash,snapshot,format=raw,file=OVMF_VARS.fd \ > -debugcon file:debug.log \ > -global isa-debugcon.iobase=0x402 \ > -chardev stdio,signal=off,mux=on,id=char0 \ > -mon chardev=char0,mode=readline \ > -serial chardev:char0 \ > -drive if=none,readonly,format=raw,file=RHEL5.11-Server-20140827.0-x86_64-DVD.iso,id=cdrom0 \ > -device ide-cd,drive=cdrom0 With these, grub is booted successfully from the RHEL5 installer ISO (which does not support UEFI at all). Unfortunately, after grub loads vmlinuz + initrd, it fails with "Not enough memory to load specified image", and returns to the grub prompt. (If I double the argument to QEMU's "-m" option, to 2048, that makes no difference.) This is at least a graceful failure. (5) The last time I built OVMF with the SeaBIOS CSM, edk2 was checked out at commit 7548947d040e ("SecurityPkg/TcgPei: drop PeiReadOnlyVariable from Depex", 2018-03-10), and SeaBIOS was checked out at commit ec6cb17f8949 ("pci: enable RedHat PCI bridges to reserve additional resources on PCI init", 2017-09-14). I've retested that combination now (in place of (3a) and (2a), respectively), and the results are identical to those from (4) (i.e. what I'm getting with the current master HEADs). I didn't experiment with different QEMU releases, or different versions of the "pc" (i440fx) machine type. I don't think it's on Hao to determine an (edk2 base commit, SeaBIOS base commit) pair from the past at which the CSM functionality worked "flawlessly". Thanks Laszlo