Re: [PATCH V5 17/33] OvmfPkg: Update Sec to support Tdx

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

  Hi,

> PcdTdxAcceptPageSize is added for page accepting. Currently TDX supports
> 4K and 2M accept page size. The default value is 4K.

Is there a good reason to not use 2M by default?  In case that fails the
code will fallback to 4K pages, so there isn't an obvious reason for
that ...

> PcdUse1GPageTable is set to FALSE by default in OvmfPkgX64.dsc. It gives
> no chance for Intel TDX to support 1G page table. To support 1G page
> table this PCD is set to TRUE in OvmfPkgX64.dsc.

Hmm, does this PCD allow using 1G pages (when supported), or does it
require 1G page support?

> +  } else if (AcceptPageSize == SIZE_2MB) {
> +    //
> +    // Total length is bigger than 2M and Page Accept size 2M is supported.
> +    //
> +    if ((PhysicalAddress & ALIGNED_2MB_MASK) == 0) {
> +      //
> +      // Start address is 2M aligned
> +      //
> +      StartAddress2 = PhysicalAddress;
> +      Length2       = TotalLength & ~(UINT64)ALIGNED_2MB_MASK;
> +
> +      if (TotalLength > Length2) {
> +        //
> +        // There is remaining part 3)
> +        //
> +        StartAddress3 = StartAddress2 + Length2;
> +        Length3       = TotalLength - Length2;
> +        ASSERT (Length3 < SIZE_2MB);
> +      }

I think I has some ideas to simplify all that math on the previous
version of this series ...

> @@ -756,13 +772,20 @@ SecCoreStartupWithStack (
>    // we use a loop rather than CopyMem.
>    //
>    IdtTableInStack.PeiService = NULL;
> +
>    for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index++) {
> -    UINT8  *Src;
> -    UINT8  *Dst;
> -    UINTN  Byte;
> +    //
> +    // Declare the local variables that actually move the data elements as
> +    // volatile to prevent the optimizer from replacing this function with
> +    // the intrinsic memcpy()
> +    //
> +    CONST UINT8     *Src;
> +    volatile UINT8  *Dst;
> +    UINTN           Byte;
> +
> +    Src = (CONST UINT8 *)&mIdtEntryTemplate;
> +    Dst = (volatile UINT8 *)&IdtTableInStack.IdtTable[Index];
>  
> -    Src = (UINT8 *)&mIdtEntryTemplate;
> -    Dst = (UINT8 *)&IdtTableInStack.IdtTable[Index];
>      for (Byte = 0; Byte < sizeof (mIdtEntryTemplate); Byte++) {
>        Dst[Byte] = Src[Byte];
>      }

This looks like an unrelated bugfix,  Move to separate patch?

take care,
  Gerd