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 <cfont...@suse.de>
>> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
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 <kra...@redhat.com>
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/seab...@seabios.org/message/BHK67ULKJTLJT676J4D5C2ND53CFEL3Q/
"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
