diff mbox series

[v2,22/31] OvmfPkg/XenPlatformPei: Rework memory detection

Message ID 20190409110844.14746-23-anthony.perard@citrix.com (mailing list archive)
State New, archived
Headers show
Series Specific platform to run OVMF in Xen PVH and HVM guests | expand

Commit Message

Anthony PERARD April 9, 2019, 11:08 a.m. UTC
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>
---

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

Comments

Laszlo Ersek April 12, 2019, 11:15 a.m. UTC | #1
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;
>      }
>    }
>  }
>
Anthony PERARD April 15, 2019, 1:42 p.m. UTC | #2
On Fri, Apr 12, 2019 at 01:15:48PM +0200, Laszlo Ersek wrote:
> On 04/09/19 13:08, Anthony PERARD wrote:
> > 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.
> > ---
> > 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.

Sorry to have cause you fear. I wasn't suggesting to make changes to
code that can run on something else than Xen. That notes was really
about code that only run on Xen, because the patch removes one thing
that OVMF do on Xen, a MtrrSetMemoryAttribute(). Also, I'll need to have
Xen-folks to answer the question.

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

Changes would be limited to XenPlatformPei, I wouldn't need to change
the code that runs on QEMU.

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

I'm glad it's useful to have a separated XenPlatformPei module.

Thanks,
diff mbox series

Patch

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;
     }
   }
 }