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

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

On 1/10/23 09:21, Gerd Hoffmann wrote:
> Add PlatformReservationConflictCB() callback function for use with
> PlatformScanE820().  It checks 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,

(1) s/virtal/virtual/

> because the AMD IOMMU uses an address window just below 1GB.

(2) s/1GB/1TB/?

> 
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 41 +++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 83a219581a1b..f12d48cad755 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -202,6 +202,46 @@ PlatformAddHobCB (
>    }
>  }
>  
> +/**
> +  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.

> +**/
> +VOID
> +PlatformReservationConflictCB (

(4) should be STATIC etc, maybe use E820 in the name rathern than
Platform, etc... up to you.

> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  IntersectionBase = MAX (
> +                               E820Entry->BaseAddr,
> +                               PlatformInfoHob->PcdPciMmio64Base
> +                               );
> +  UINT64  IntersectionEnd = MIN (
> +                              E820Entry->BaseAddr + E820Entry->Length,
> +                              PlatformInfoHob->PcdPciMmio64Base +
> +                              PlatformInfoHob->PcdPciMmio64Size
> +                              );

(5) Locals should not be initialized (per my last memories of the edk2
coding style).

> +  UINT64  NewBase;
> +
> +  if (IntersectionBase >= IntersectionEnd) {
> +    return;  // no overlap
> +  }
> +
> +  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
> +             PlatformInfoHob->PcdPciMmio64Size);

(6) This appears a typo; we'll want

  NewBase + PcdPciMmio64Size == E820Entry->BaseAddr

where the LHS stands for the exclusive end of the moved MMIO aperture,
with unchanged size, and the RHS stands for the inclusive base of the
(unmovable) reservation. The above formula ensures that the intersection
be empty, without changing sizes, or moving the reservation. Then,
reordering the formula for an assignment, we get

  NewBase = E820Entry->BaseAddr - PcdPciMmio64Size;

as an assignment.

So, yes, a typo.

> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: move mmio: 0x%Lx => %Lx\n",

(7) pls consider DEBUG_VERBOSE, like before

(8) usage of the 0x prefix in the debug message is inconsistent, we
should prefix the NewBase output with it, too

> +    __FUNCTION__,
> +    PlatformInfoHob->PcdPciMmio64Base,
> +    NewBase
> +    ));
> +  PlatformInfoHob->PcdPciMmio64Base = NewBase;
> +}

(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.

> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
>    passed callback for each entry.
> @@ -638,6 +678,7 @@ PlatformDynamicMmioWindow (
>      DEBUG ((DEBUG_INFO, "%a:   MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
>      PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
>      PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
> +    PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob);
>    } else {
>      DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
>    }

Looks fine.

Thanks,
Laszlo