Re: [edk2-devel] [PATCH V2 05/28] MdePkg: Add TdxLib to wrap Tdx operations

["Gerd Hoffmann" <kraxel@redhat.com>]

> +// PageSize is mapped to PageLevel like below:
> +// 4KB - 0, 2MB - 1
> +UINT64  mTdxAcceptPageLevelMap[2] = {
> +  SIZE_4KB,
> +  SIZE_2MB

No 1G pages?

> +UINTN
> +GetGpaPageLevel (
> +  UINT64 PageSize

Uh, UINT32 is not enough?  Keep the door open for 512G pages?

> +{
> +  UINTN Index;
> +
> +  for (Index = 0; Index < sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0]); Index++) {

There is an ARRAY_SIZE() macro, no need to open code the sizeof() trick.

> +    if (mTdxAcceptPageLevelMap[Index] == PageSize) {
> +      break;
> +    }
> +  }
> +
> +  return Index;
> +}

No error handling (invalid PageSize) here?

> +/**
> +  This function accept a pending private page, and initialize the page to
> +  all-0 using the TD ephemeral private key.
> +
> +  Sometimes TDCALL [TDG.MEM.PAGE.ACCEPT] may return
> +  TDX_EXIT_REASON_PAGE_SIZE_MISMATCH. It indicates the input PageLevel is
> +  not workable. In this case we need to try to fallback to a smaller
> +  PageLevel if possible.
> +
> +  @param[in]  StartAddress      Guest physical address of the private
> +                                page to accept.
> +  @param[in]  NumberOfPages     Number of the pages to be accepted.
> +  @param[in]  PageSize          GPA page size. Only accept 1G/2M/4K size.
> +
> +  @return EFI_SUCCESS           Accept successfully
> +  @return others                Indicate other errors
> +**/
> +EFI_STATUS
> +EFIAPI
> +TdAcceptPages (
> +  IN UINT64  StartAddress,
> +  IN UINT64  NumberOfPages,
> +  IN UINT64  PageSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT64      Address;
> +  UINT64      TdxStatus;
> +  UINT64      Index;
> +  UINT64      GpaPageLevel;
> +  UINT64      PageSize2;
> +
> +  Address = StartAddress;
> +
> +  GpaPageLevel = (UINT64) GetGpaPageLevel (PageSize);

Why cast?

> +  if (GpaPageLevel > sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0])) {
> +    DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size - 0x%llx\n", PageSize));

Ah.  Errors are catched here.  Well, no.  The check is wrong,
it should be ">=" not ">".

Better would be GetGpaPageLevel explicitly returning a specific value
(for example -1) on error.

> +  Status = EFI_SUCCESS;
> +  for (Index = 0; Index < NumberOfPages; Index++) {
> +    TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0, 0);
> +    if (TdxStatus != TDX_EXIT_REASON_SUCCESS) {
> +        if ((TdxStatus & ~0xFFFFULL) == TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) {
> +          //
> +          // Already accepted
> +          //
> +          mNumberOfDuplicatedAcceptedPages++;

Hmm.  When this happens we have a bug somewhere, right?
So should this be an assert()?
Or should we at least log the address?

> +#define RTMR_COUNT                  4
> +#define TD_EXTEND_BUFFER_LEN        (64 + 64)
> +#define EXTEND_BUFFER_ADDRESS_MASK  0x3f
> +
> +
> +#pragma pack(16)
> +typedef struct {
> +  UINT8   Buffer[TD_EXTEND_BUFFER_LEN];
> +} TDX_EXTEND_BUFFER;
> +#pragma pack()
> +
> +UINT8               *mExtendBufferAddress = NULL;
> +TDX_EXTEND_BUFFER   mExtendBuffer;
> +
> +/**
> +  TD.RTMR.EXTEND requires 64B-aligned guest physical address of
> +  48B-extension data. In runtime we walk thru the Buffer to find
> +  out a 64B-aligned start address.

Can't you just use __attribute__((aligned(64))) for that?

take care,
  Gerd