[SeaBIOS] Re: [PATCH v5] limit physical address space size

[Claudio Fontana]

On 11/8/23 19:35, Kevin O'Connor wrote:
> On Tue, Nov 07, 2023 at 02:03:09PM +0100, Gerd Hoffmann wrote:
> > For better compatibility with old linux kernels,
> > see source code comment.
> >
> > Related (same problem in ovmf):
> > https://github.com/tianocore/edk2/commit/c1e853769046
>
> Thanks.  I'll defer to your judgement on this.  It does seem a little
> odd to alter the CPUPhysBits variable itself instead of adding
> additional checking to the pciinit.c code that uses CPUPhysBits.
> (That is, if there are old Linux kernels that can't handle a very high
> PCI space, then a workaround in the PCI code might make it more clear
> what is occurring.)
>
> Cheers,
> -Kevin
> > Cc: Claudio Fontana 
> > Signed-off-by: Gerd Hoffmann

Hi,

I thought about this, and I am not sure it's the right call though.

The change breaking old guests (CentOS7, Ubuntu 18.04 are the known ones, but presumably others as well) in QEMU is:

commit 784155cdcb02ffaae44afecab93861070e7d652d
Author: Gerd Hoffmann 
Date:   Mon Sep 11 17:20:26 2023 +0200

seabios: update submodule to git snapshot

git shortlog
    ------------

Gerd Hoffmann (7):
          disable array bounds warning
          better kvm detection
          detect physical address space size
          move 64bit pci window to end of address space
          be less conservative with the 64bit pci io window
          qemu: log reservations in fw_cfg e820 table
          check for e820 conflict

The reasoning for the change is according to:

https://mail.coreboot.org/hyperkitty/list/[e-mail]/message/BHK67U...

"It makes seabios follow a common pattern.  real mode address space
has io resources mapped high (below 1M).  32-bit address space has
io resources mapped high too (below 4G).  This does the same for
64-bit resources."

So it seems to be an almost aesthetic choice, the way I read the "common pattern", one that ends up actually breaking existing workloads though.

Now the correction to that that you propose in SeaBIOS is:
> > src/fw/paravirt.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> > index e5d4eca0cb5a..a2c9c64d5e78 100644
> > --- a/src/fw/paravirt.c
> > +++ b/src/fw/paravirt.c
> > @@ -182,6 +182,14 @@ static void physbits(int qemu_quirk)
> >              __func__, signature, pae ? "yes" : "no", CPULongMode ? "yes" : "no",
> >              physbits, valid ? "yes" : "no");
> > if (valid && physbits > 46) {
> >    // Old linux kernels have trouble dealing with more than 46
> >    // phys-bits, so avoid that for now.  Seems to be a bug in the
> >    // virtio-pci driver.  Reported: centos-7, ubuntu-18.04
> >    dprintf(1, "%s: using phys-bits=46 (old linux kernel compatibility)\n", __func__);
> >    physbits = 46;
> > }
> > if (valid)
> >        CPUPhysBits = physbits;
> > }
> >
> > 2.41.0

but to me this is potentially breaking the situation for another set of users,
those that are passing through 48 physical bits from their host cpu to the guest,
and expect it to actually happen.

Would it not be a better solution to instead either revert the original change,
or to patch it to find a new range that better satisfies code consistency/aesthetic requirements,
but also does not break any running workload?

I could help with the testing for this.

Or we could add some extra workarounds in the stack elsewhere in the management tools to
try to detect older guests (not ideal either), but this seems like adding breakage on top of breakage to me.

Might be wrong though, looking forward for opinions across Seabios and QEMU.

Thanks,

Claudio