Re: [edk2-devel] [PATCH V4 22/31] OvmfPkg: Update PlatformPei to support TDX

["Min Xu" <min.m.xu@intel.com>]

Hi
> > +/**
> > +  Check if it is Tdx guest
> > +
> > +  @retval    TRUE   It is Tdx guest
> > +  @retval    FALSE  It is not Tdx guest
> > +**/
> > +BOOLEAN
> > +PlatformPeiIsTdxGuest (
> > +  VOID
> > +  )
> > +{
> > +  CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER
> *CcWorkAreaHeader;
> > +
> > +  CcWorkAreaHeader =
> (CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER
> > +*)FixedPcdGet32 (PcdOvmfWorkAreaBase);
> > +  return (CcWorkAreaHeader != NULL && CcWorkAreaHeader->GuestType
> ==
> > +GUEST_TYPE_INTEL_TDX); }
> 
> fwcfg patch adds a similar function.  Can't we have a single function
> somewhere, or have a #define for this check, instead of doing this
> cut+paste programming?
There is TdIsEnabled() in BaseLib. So PlatformPeiIsTdxGuest is deprecated in the next version.
> 
> > +VOID
> > +EFIAPI
> > +DEBUG_HOBLIST (
> > +  IN CONST VOID  *HobStart
> > +  )
> > +{
> > +  EFI_PEI_HOB_POINTERS  Hob;
> > +
> > +  Hob.Raw = (UINT8 *)HobStart;
> > +  //
> > +  // Parse the HOB list until end of list or matching type is found.
> > +  //
> > +  while (!END_OF_HOB_LIST (Hob)) {
> > +    DEBUG ((DEBUG_INFO, "HOB(%p) : %x %x\n", Hob, Hob.Header-
> >HobType, Hob.Header->HobLength));
> > +    switch (Hob.Header->HobType) {
> > +      case EFI_HOB_TYPE_RESOURCE_DESCRIPTOR:
> > +        DEBUG ((
> > +          DEBUG_INFO,
> > +          "\t: %x %x %llx %llx\n",
> > +          Hob.ResourceDescriptor->ResourceType,
> > +          Hob.ResourceDescriptor->ResourceAttribute,
> > +          Hob.ResourceDescriptor->PhysicalStart,
> > +          Hob.ResourceDescriptor->ResourceLength
> > +          ));
> > +
> > +        break;
> > +      case EFI_HOB_TYPE_MEMORY_ALLOCATION:
> > +        DEBUG ((
> > +          DEBUG_INFO,
> > +          "\t: %llx %llx %x\n",
> > +          Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress,
> > +          Hob.MemoryAllocation->AllocDescriptor.MemoryLength,
> > +          Hob.MemoryAllocation->AllocDescriptor.MemoryType
> > +          ));
> > +        break;
> > +      default:
> > +        break;
> > +    }
> > +
> > +    Hob.Raw = GET_NEXT_HOB (Hob);
> > +  }
> > +}
> 
> Likewise, I've seen this before in another patch of this series.
This will be deprecated in the next version.
> 
> > diff --git a/OvmfPkg/PlatformPei/MemDetect.c
> > b/OvmfPkg/PlatformPei/MemDetect.c index 934d5c196570..9227fa260ccd
> > 100644
> > --- a/OvmfPkg/PlatformPei/MemDetect.c
> > +++ b/OvmfPkg/PlatformPei/MemDetect.c
> > @@ -36,6 +36,7 @@ Module Name:
> >  #include <Library/MtrrLib.h>
> >  #include <Library/QemuFwCfgLib.h>
> >  #include <Library/QemuFwCfgSimpleParserLib.h>
> > +#include <Library/TdxLib.h>
> >
> >  #include "Platform.h"
> >  #include "Cmos.h"
> > @@ -556,7 +557,19 @@ AddressWidthInitialization (
> >      mPhysMemAddressWidth = 36;
> >    }
> >
> > + #if defined (MDE_CPU_X64)
> > +  if (PlatformPeiIsTdxGuest ()) {
> > +    if (TdSharedPageMask () == (1ULL << 47)) {
> > +      mPhysMemAddressWidth = 48;
> > +    } else {
> > +      mPhysMemAddressWidth = 52;
> > +    }
> > +  }
> > +
> > +  ASSERT (mPhysMemAddressWidth <= 52); #else
> 
> Making this TDX-specific looks wrong to me.  5-level paging exists outside TDX
> too.
> 
> Given we don't support 5-level paging (yet) I think we can just drop this and
> revisit in case 5-level paging support is added in the future.
> 
> > +  UINT32   Pml5Entries;
> 
> Same here.
mPhysMemAddressWidth indicates the physical memory address width. Tdx guest supports GPAW of 52 or 48. So I think here mPhysMemAddresWidth is correct.
Even mPhysMemAddress is 52, we can still use 4-level paging. So I am going to update GetPeiMemoryCap() like below:

  if (mPhysMemAddressWidth <= 39) {
    Pml4Entries = 1;
    PdpEntries = 1 << (mPhysMemAddressWidth - 30);
    ASSERT (PdpEntries <= 0x200);
  } else {
    if (TdIsEnabled ()) {           <-- If it is td guest, then Pml4Entries is set to 0x200.
      Pml4Entries = 0x200;
    } else {
      Pml4Entries = 1 << (mPhysMemAddressWidth - 39);
    }
    ASSERT (Pml4Entries <= 0x200);
    PdpEntries = 512;
  }

Thanks
Min