Re: [PATCH v4 23/35] OvmfPkg/XenPlatformPei: Rework memory detection

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jul 29, 2019 at 04:39:32PM +0100, 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
> work without CMOS by reading the e820 table.
> 
> Rework XenPublishRamRegions to also care for the reserved and ACPI
> entry in the e820 table. The region that was added by InitializeXen()
> isn't needed as that same entry is in the e820 table provided by
> hvmloader.
> 
> MTRR settings aren't modified anymore, on HVM it's already done by
> hvmloader, on PVH it is supposed to have sane default. MTRR will need
> to be done properly but keeping what's already been done by programmes
                                                              ^ firmware
> that has runned before OVMF will do for now.
           ^ ran

> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index a21d657357..182e96cc5b 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -276,9 +276,12 @@ XenPublishRamRegions (
>    VOID
>    )
>  {
> -  EFI_E820_ENTRY64  *E820Map;
> -  UINT32            E820EntriesCount;
> -  EFI_STATUS        Status;
> +  EFI_E820_ENTRY64      *E820Map;
> +  UINT32                E820EntriesCount;
> +  EFI_STATUS            Status;
> +  EFI_E820_ENTRY64      *Entry;
> +  UINTN                 Index;
> +  EFI_PHYSICAL_ADDRESS  LocalApic;
>  
>    DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n"));
>  
> @@ -287,26 +290,47 @@ XenPublishRamRegions (
>    //
>    E820EntriesCount = 0;
>    Status = XenGetE820Map (&E820Map, &E820EntriesCount);
> -
>    ASSERT_EFI_ERROR (Status);
>  
> -  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;
> +  LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress);
> +  AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB);
> +
> +  for (Index = 0; Index < E820EntriesCount; Index++) {
> +    UINT64 Base;
> +    UINT64 End;
> +
> +    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:
> +      AddReservedMemoryRangeHob (Base, End, FALSE);
> +      break;
> +    case EfiAcpiAddressRangeReserved:
> +      if (Base < LocalApic && LocalApic < End) {

Don't you also need to check for equality? In case such region starts
at Base == LocalApic?

I guess it doesn't matter that much since this is to workaround a
specific issue with hvmloader, but I would like to see this sorted out
in hvmloader so that there's no clash anymore.

Thanks, Roger.