Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB

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

On 1/11/23 09:26, Gerd Hoffmann wrote:
>   Hi,
> 
>>> +/**
>>> +  Check whenever the 64bit PCI MMIO window overlaps with a reservation
>>> +  from qemu.  If so move down the MMIO window to resolve the conflict.
>>> +
>>> +  This happens on (virtal) AMD machines with 1TB address space,
>>> +  because the AMD IOMMU uses an address window just below 1GB.
>>
>> (3) Same two typos, I think, as in the commit message.
> 
> Duplicated by the power of cut + paste.
> 
>>> +  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
>>> +             PlatformInfoHob->PcdPciMmio64Size);
>>
>> (6) This appears a typo; we'll want
>>
>>   NewBase + PcdPciMmio64Size == E820Entry->BaseAddr
> 
> But then NewBase is not aligned.  The assignment above moves it down
> while maintaining the existing alignment.

Ah, good point. I totally missed the idea here.

The starting predicate is, apparently, that the base is aligned by size
("naturally aligned"), so by subtracting size from the aperture, we get
a new aperture that conforms to both properties below:

- still naturally aligned (original predicate preserved),

- empty intersection with the original aperture (because the new
aperture will end where the old one just started).

Now, here's one thought: just because the new (sunken) aperture does not
intersect the pre-move aperture, is it guaranteed to also steer clear of
the E820 Entry either?

I don't think that's certain. If the E820 Entry overlaps the bottom of
the original aperture, but goes "deeper" than that, then it will overlap
the top of the sunken aperture too.

So I think we might want to do a two step process here, in case the
original aperture overlaps the E820 Entry:

- push down the aperture (same size) so its exclusive end equal the
inclusive start of the E820 Entry

- align *down* (truncate) the base of the aperture, while keeping its
size unchanged.

Something like:

  NewBase = E820Entry->BaseAddr - PcdPciMmio64Size;

  ASSERT (PcdPciMmio64Size != 0);
  ASSERT ((PcdPciMmio64Size & (PcdPciMmio64Size - 1)) == 0);
  NewBase = NewBase & ~(PcdPciMmio64Size - 1);

> 
>> (9) Do we need any other checks or maybe assertions that we're only
>> conflicting with a reserved area, and/or that the subtraction for
>> NewBase does not underflow?
>>
>> I don't think we can "armor" this very well, I'm just pondering if there
>> are any egregious misunderstandings between QEMU and the firmware that
>> we might want to catch here. If not, that's OK of course.
> 
> Yes, it's hard to design something which can handle all reservations
> qemu might do in the future correctly.  And, yes, the code above works
> because we know the qemu reservation is smaller than the mmio window, so
> moving down to the next naturally aligned address actually solves the
> conflict.

Not only smaller, but also *not encompassing* the lower boundary of the
MMIO window.

Anyway, up to you -- if you want to stick with the logic shown in this
patch, I'm OK, but then please add some comments, or maybe even some
ASSERTs.

Thanks!
Laszlo