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

[Laszlo Ersek <lersek@redhat.com>]

On 01/10/17 17:18, Anthony PERARD wrote:
> 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.
> 

Right, that would be best.

In fact, this affects two modules, (Xen)PlatformPei and
PlatformBootManagerLib. In both, we already have the XenDetected()
function. Maybe you can add a XenPvhDetected() function as well (which
would return TRUE only for PVH, while the original XenDetected() would
continue returning TRUE for both HVM and PVH). Then the PCI host bridge
ID checking could be entirely short-circuited if XenPvhDetected()
returned TRUE first.

Thanks
Laszlo