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

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote:
> 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.

This value is just the result of a read to an incorrect location, and
the return value is -1. There is no PCI when running in Xen PVH, at
least for now. I think I should try harder to find out if OVMF is
running in PVH, and so I would not need this case 0xffff at all.

-- 
Anthony PERARD