Re: [edk2-devel] [PATCH v2 22/31] OvmfPkg/XenPlatformPei: Rework memory detection

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

On 04/09/19 13:08, Anthony PERARD wrote:
> When running as a Xen PVH guest, there is no CMOS to read the memory
> size from.  Rework GetSystemMemorySize(Below|Above)4gb() so they can
> works without CMOS by reading the e820 table.
> 
> Rework XenPublishRamRegions for PVH, handle the Reserve type and explain
> about the ACPI type. MTRR settings aren't modified anymore, on HVM, it's
> already done by hvmloader, on PVH it is supposed to have sane default.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---

For this patch:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Another comment below:

> 
> Notes:
>     About MTRR, should we redo the setting in OVMF? Even if in both case of
>     PVH and HVM, something would have setup the default type to write back
>     and handle a few other ranges like PCI hole, hvmloader for HVM or and
>     libxc I think for PVH.

This patch is *exactly* the kind of change that I want to keep as far as
possible away from code that runs (even if in part) on QEMU. Every time
I need to touch OvmfPkg/PlatformPei/MemDetect.c, and in particular go
near the MTRR setup or the physical memory layout (resource descriptor
HOBs, CPU address width, etc), I start convulsing.

If, under "OvmfPkg/PlatformPei/", you could limit your changes to
"Xen.c", I'd be OK with that. Otherwise, please don't go near that code.

Again, this is *the* kind of change why we have the platform split /
duplication.

Thanks
Laszlo

>     
>     (For PVH, it's in the spec as well
>     https://xenbits.xenproject.org/docs/unstable/misc/pvh.html#mtrr )
> 
>  OvmfPkg/XenPlatformPei/Platform.h  |  6 ++
>  OvmfPkg/XenPlatformPei/MemDetect.c | 71 ++++++++++++++++++++
>  OvmfPkg/XenPlatformPei/Xen.c       | 47 +++++++++----
>  3 files changed, 111 insertions(+), 13 deletions(-)
> 
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index c5a139f016..d6d93eab2d 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -120,6 +120,12 @@ XenPublishRamRegions (
>    VOID
>    );
>  
> +EFI_STATUS
> +XenGetE820Map (
> +  EFI_E820_ENTRY64 **Entries,
> +  UINT32 *Count
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
> index db3d387d1c..0c066d518b 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -102,6 +102,47 @@ Q35TsegMbytesInitialization (
>    mQ35TsegMbytes = ExtendedTsegMbytes;
>  }
>  
> +STATIC
> +UINT64
> +GetHighestSystemMemoryAddress (
> +  BOOLEAN       Below4gb
> +  )
> +{
> +  EFI_E820_ENTRY64    *E820Map;
> +  UINT32              E820EntriesCount;
> +  EFI_E820_ENTRY64    *Entry;
> +  EFI_STATUS          Status;
> +  UINT32              Loop;
> +  UINT64              HighestAddress;
> +  UINT64              EntryEnd;
> +
> +  HighestAddress = 0;
> +
> +  Status = XenGetE820Map (&E820Map, &E820EntriesCount);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  for (Loop = 0; Loop < E820EntriesCount; Loop++) {
> +    Entry = E820Map + Loop;
> +    EntryEnd = Entry->BaseAddr + Entry->Length;
> +
> +    if (Entry->Type == EfiAcpiAddressRangeMemory &&
> +        EntryEnd > HighestAddress) {
> +
> +      if (Below4gb && (EntryEnd <= BASE_4GB)) {
> +        HighestAddress = EntryEnd;
> +      } else if (!Below4gb && (EntryEnd >= BASE_4GB)) {
> +        HighestAddress = EntryEnd;
> +      }
> +    }
> +  }
> +
> +  //
> +  // Round down the end address.
> +  //
> +  HighestAddress &= ~(UINT64)EFI_PAGE_MASK;
> +
> +  return HighestAddress;
> +}
>  
>  UINT32
>  GetSystemMemorySizeBelow4gb (
> @@ -111,6 +152,19 @@ GetSystemMemorySizeBelow4gb (
>    UINT8 Cmos0x34;
>    UINT8 Cmos0x35;
>  
> +  //
> +  // In PVH case, there is no CMOS, we have to calculate the memory size
> +  // from parsing the E820
> +  //
> +  if (XenPvhDetected ()) {
> +    UINT64  HighestAddress;
> +
> +    HighestAddress = GetHighestSystemMemoryAddress (TRUE);
> +    ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB);
> +
> +    return HighestAddress;
> +  }
> +
>    //
>    // CMOS 0x34/0x35 specifies the system memory above 16 MB.
>    // * CMOS(0x35) is the high byte
> @@ -135,6 +189,23 @@ GetSystemMemorySizeAbove4gb (
>    UINT32 Size;
>    UINTN  CmosIndex;
>  
> +  //
> +  // In PVH case, there is no CMOS, we have to calculate the memory size
> +  // from parsing the E820
> +  //
> +  if (XenPvhDetected ()) {
> +    UINT64  HighestAddress;
> +
> +    HighestAddress = GetHighestSystemMemoryAddress (FALSE);
> +    ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB);
> +
> +    if (HighestAddress >= BASE_4GB) {
> +      HighestAddress -= BASE_4GB;
> +    }
> +
> +    return HighestAddress;
> +  }
> +
>    //
>    // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
>    // * CMOS(0x5d) is the most significant size byte
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index f8cee671d8..7b503f2c4e 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -282,6 +282,8 @@ XenPublishRamRegions (
>    EFI_E820_ENTRY64  *E820Map;
>    UINT32            E820EntriesCount;
>    EFI_STATUS        Status;
> +  EFI_E820_ENTRY64 *Entry;
> +  UINTN Index;
>  
>    DEBUG ((EFI_D_INFO, "Using memory map provided by Xen\n"));
>  
> @@ -290,26 +292,45 @@ XenPublishRamRegions (
>    //
>    E820EntriesCount = 0;
>    Status = XenGetE820Map (&E820Map, &E820EntriesCount);
> -
>    ASSERT_EFI_ERROR (Status);
>  
> -  if (E820EntriesCount > 0) {
> -    EFI_E820_ENTRY64 *Entry;
> -    UINT32 Loop;
> +  for (Index = 0; Index < E820EntriesCount; Index++) {
> +    UINT64 Base;
> +    UINT64 End;
>  
> -    for (Loop = 0; Loop < E820EntriesCount; Loop++) {
> -      Entry = E820Map + Loop;
> +    Entry = &E820Map[Index];
>  
> +
> +    //
> +    // Round up the start address, and round down the end address.
> +    //
> +    Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE);
> +    End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK;
> +
> +    switch (Entry->Type) {
> +    case EfiAcpiAddressRangeMemory:
> +      AddMemoryRangeHob (Base, End);
> +      break;
> +    case EfiAcpiAddressRangeACPI:
> +      //
> +      // Ignore, OVMF should read the ACPI tables and provide them to linux
> +      // from a different location.
> +      //
> +      break;
> +    case EfiAcpiAddressRangeReserved:
>        //
> -      // Only care about RAM
> +      // Avoid ranges marked as reserved in the e820 table provided by
> +      // hvmloader as it conflicts with an other aperture.
> +      // error message: CpuDxe: IntersectMemoryDescriptor:
> +      //        desc [FC000000, 100000000) type 1 cap 8700000000026001
> +      //        conflicts with aperture [FEE00000, FEE01000) cap 1
>        //
> -      if (Entry->Type != EfiAcpiAddressRangeMemory) {
> -        continue;
> +      if (!XenHvmloaderDetected ()) {
> +        AddReservedMemoryBaseSizeHob (Base, End - Base, FALSE);
>        }
> -
> -      AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length);
> -
> -      MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack);
> +      break;
> +    default:
> +      break;
>      }
>    }
>  }
>