Re: [PATCH v2 0/6] Ovmf: Drop IntelFramework[Module]Pkg dependency

["Laszlo Ersek" <lersek@redhat.com>]

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