Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB

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

On 1/11/23 16:23, Gerd Hoffmann wrote:

> Some test case complained because the 80000000-afffffff range is io
> address space (according to /proc/iomem) but not tagged as uncachable
> in mtrr registers.

Ah, very interesting! I didn't know there was a test case for this.

(And now I'm curious, per the new BZ, whether this same test case
complained if it saw us go deeper with the low mem amount -- e.g. the
same situation arises between 0x7000_0000 and 0x8000_0000.)

>>> With gigabyte-alignment being the common case these days it might make
>>> sense to place the MMCONFIG area at 0xe0000000 instead ...
>>
>> I feel really unsafe about complicating this code even further.
> 
> I think it should actually simplify things.  All the inconsistencies we
> have (as you outlined above) due to the hole punching and edk2
> supporting only a single range for 32bit mmio should go away, and we
> will have less address space layout differences between q35 and pc.

We've tried 0xE000_0000 in the past, in commit 75136b29541b.

But had to revert it in commit eb4d62b0779c, due to 0xE000_0000 tickling
a bug in QEMU.

The bug tickling was actually reported by you :) See
<https://bugzilla.tianocore.org/show_bug.cgi?id=1859>.

The current 0xB000_0000 value comes from commit b07de0974b65a, which was
a replacement for 75136b29541b (after the revert -- for re-fixing the
original issue <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>,
which 75136b29541b intended to fix in the first place).

> 
> We'll set LowMemory  -> 4G to UC via mtrr (both pc and q35)

This is problematic, as LowMemory can have any kind of "resolution"
(i.e. an effectively unrestricted count of 1-bits in its binary
representation). That's a problem because MtrrLib's algorithm (probably
justifiedly) cannot find enough variable MTRRs to cover the boundary
precisely -- and will fail.

This is why our current logic exists for i440fx, in
PlatformQemuUc32BaseInitialization(), rounding up Uc32Base from LowMemory.

(Well, if you mean to keep the same logic for both i440fx adn q35,
that's OK then.)

> 
> We'll use LowMemory  -> 0xFBFFFFFF (pc) or
>           LowMemory  -> 0xdfffffff (q35) as 32bit mmio window.

This is precisely the situation (32-bit MMIO aperture below EXBAR) that
triggered the QEMU bug described in TianoCore#1859 and commit eb4d62b0779c.

I don't know if that QEMU bug is now fixed (and how far in the QEMU
release history the breakage goes back). At the time of the edk2 revert
commit eb4d62b0779c (2019-JUN-03), QEMU's latest release was v4.0.0
(2019-APR-23). Release v4.1.0 would follow at 2019-AUG-15.

> We'll use 0xe0000000 -> 0xeffffffff for mmconfig (q35 only).
> 
> Qemu will add 0xf0000000 -> 0xFBFFFFFF to the PCI bus _CRS so
> linux could use it but the firmware wouldn't do anything with
> it (q35 only).

If QEMU only *added* this range to the _CRS, that would be fine, but the
QEMU issue in TianoCore#1859 was that QEMU *moved* the aperture to this
range in the _CRS, effectively invalidating all the firmware-assigned
BARs (which would now all fall outside of the _CRS).

If you can ascertain that this problem is no longer relevant in any QEMU
releases that are still in use, then I won't try to block this
direction. In that case, it might be sufficient to just "re-play" commit
75136b29541b. (Note however that the MTRR setup was tied to the approach
taken, please refer to commits 39b9a5ffe661 and 49edde15230a.)

Either way, this has been a brittle area of OVMF platform code, and I'd
feel real uncomfortable, providing an explicit ACK. ... Luckily, you
wouldn't need an ACK from me :)

Laszlo