Re: [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code

[Laszlo Ersek <lersek@redhat.com>]

On 12/08/16 16:33, Anthony PERARD wrote:
> - learn about memory size from the E820
> - ignore error if host bridge devid is 0xffff, PVH does not have PCI and
>   reading from unexisting device return -1.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenPlatformPei/MemDetect.c | 71 ++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenPlatformPei/Platform.c  |  5 +++
>  OvmfPkg/XenPlatformPei/Platform.h  | 10 ++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
> index 4ecdf5e..0d80775 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth;
>  STATIC UINT32 mS3AcpiReservedMemoryBase;
>  STATIC UINT32 mS3AcpiReservedMemorySize;
>  
> +STATIC UINT32 mXenLowerMemorySize = 0;
> +STATIC UINT64 mXenHighMemorySize = 0;
> +
> +VOID
> +XenReadMemorySizes (
> +  VOID
> +  )
> +{
> +  EFI_E820_ENTRY64  *E820Map;
> +  UINT32            E820EntriesCount;
> +  EFI_STATUS Status;
> +
> +  Status = XenGetE820Map (&E820Map, &E820EntriesCount);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  mXenLowerMemorySize = 0;
> +  mXenHighMemorySize = 0;
> +
> +  if (E820EntriesCount > 0) {
> +    EFI_E820_ENTRY64 *Entry;
> +    UINT32 Loop;
> +
> +    for (Loop = 0; Loop < E820EntriesCount; Loop++) {
> +      Entry = E820Map + Loop;
> +
> +      //
> +      // Only care about RAM
> +      //
> +      if (Entry->Type != EfiAcpiAddressRangeMemory) {
> +        continue;
> +      }
> +
> +      if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) {
> +        continue;
> +      }
> +
> +      if (Entry->BaseAddr < SIZE_16MB) {
> +        UINT64 bottom = Entry->BaseAddr;
> +        UINT64 size = Entry->Length;
> +        size -= SIZE_16MB - bottom;
> +        bottom = SIZE_16MB;
> +        mXenLowerMemorySize += size;
> +        continue;
> +      }
> +      if (Entry->BaseAddr <= SIZE_4GB) {
> +        UINT64 size = Entry->Length;
> +        mXenLowerMemorySize += size;
> +        continue;
> +      }
> +
> +      if (Entry->BaseAddr == SIZE_4GB) {
> +        mXenHighMemorySize = Entry->Length;
> +        continue;
> +      }
> +
> +      DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n",
> +              __FUNCTION__,
> +              Entry->BaseAddr,
> +              Entry->Length));
> +    }
> +    mXenLowerMemorySize += SIZE_16MB;
> +  }
> +}
> +
>  UINT32
>  GetSystemMemorySizeBelow4gb (
>    VOID
> @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb (
>    UINT8 Cmos0x34;
>    UINT8 Cmos0x35;
>  
> +  if (mXen && mXenLowerMemorySize)
> +    return mXenLowerMemorySize;
> +
>    //
>    // CMOS 0x34/0x35 specifies the system memory above 16 MB.
>    // * CMOS(0x35) is the high byte
> @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb (
>    UINT32 Size;
>    UINTN  CmosIndex;
>  
> +  // if lower memory is specified that way, return also high memory
> +  if (mXen && mXenLowerMemorySize)
> +    return mXenHighMemorySize;
> +
>    //
>    // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
>    // * CMOS(0x5d) is the most significant size byte
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> index bf78878..9fc713c 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -362,6 +362,10 @@ MiscInitialization (
>        AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL);
>        AcpiEnBit  = ICH9_ACPI_CNTL_ACPI_EN;
>        break;
> +    case 0xffff:
> +      // xen PVH, ignore
> +      PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> +      return;

Can we create a new macro for 0xFFFF in
"OvmfPkg/Include/OvmfPlatforms.h", and use that here?

Normally I would suggest a separate header file under "OvmfPkg/Include",
similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new
file in "OvmfPlatforms.h", but here we only need a single "chipset
identifier", so I guess that can go directly into "OvmfPlatforms.h".

I mainly suggest this because in patch 08/14, the same 0xFFFF case label
is being added to code shared with QEMU, and using a verbose macro there
is much better than a magic number. In turn, we should use the same
macro here, I think.

Looks OK otherwise.

Thanks
Laszlo

>      default:
>        DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
>          __FUNCTION__, mHostBridgeDevId));
> @@ -503,6 +507,7 @@ InitializeXenPlatform (
>    DebugDumpCmos ();
>  
>    XenDetect ();
> +  XenReadMemorySizes ();
>  
>    BootModeInitialization ();
>    AddressWidthInitialization ();
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index eda765b..2948853 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -101,4 +101,14 @@ extern BOOLEAN mS3Supported;
>  
>  extern UINT8 mPhysMemAddressWidth;
>  
> +EFI_STATUS
> +XenGetE820Map (
> +  EFI_E820_ENTRY64 **Entries,
> +  UINT32 *Count
> +  );
> +VOID
> +XenReadMemorySizes (
> +  VOID
> +  );
> +
>  #endif // _PLATFORM_PEI_H_INCLUDED_
>