Re: [PATCH V2 07/28] UefiCpuPkg: Support TDX in BaseXApicX2ApicLib

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

On October 13, 2021 1:31 PM, Ray Ni wrote:
> Min,
> Comments below:
> 
> +**/
> +BOOLEAN
> +EFIAPI
> 
> 1. EFIAPI is for public lib API. Is this a public API?
No, it is not a public API. The EFIAPI will be removed. Thanks for reminder.
> 
> +BaseXApicIsTdxGuest (
> +  VOID
> +  )
> +{
> +  UINT32    Eax;
> +  UINT32    Ebx;
> +  UINT32    Ecx;
> +  UINT32    Edx;
> +  UINT32    LargestEax;
> +
> +  if (mBaseXApicTdxProbed) {
> +    return mBaseXApicIsTdxEnabled;
> +  }
> +
> +  mBaseXApicIsTdxEnabled = FALSE;
> 
> 2. ApicLib can be used in pre-mem running directly in flash.
> The global variable cannot be modified in that case.
What will happen when the global variable is modified in flash?
Will the system hang? Or just a failure of write operation?
> 
> 
> +
> +  do {
> +    AsmCpuid (0, &LargestEax, &Ebx, &Ecx, &Edx);
> 
> +
> +    if (Ebx != SIGNATURE_32 ('G', 'e', 'n', 'u')
> +      || Edx != SIGNATURE_32 ('i', 'n', 'e', 'I')
> +      || Ecx != SIGNATURE_32 ('n', 't', 'e', 'l')) {
> +      break;
> +    }
> +
> +    AsmCpuid (1, NULL, NULL, &Ecx, NULL);
> +    if ((Ecx & BIT31) == 0) {
> +      break;
> +    }
> +
> +    if (LargestEax < 0x21) {
> +      break;
> +    }
> +
> +    AsmCpuidEx (0x21, 0, &Eax, &Ebx, &Ecx, &Edx);
> +    if (Ebx != SIGNATURE_32 ('I', 'n', 't', 'e')
> +      || Edx != SIGNATURE_32 ('l', 'T', 'D', 'X')
> +      || Ecx != SIGNATURE_32 (' ', ' ', ' ', ' ')) {
> +      break;
> +    }
> 
> 
> 3. Can you use definition from MdePkg\Include\Register\Intel\Cpuid.h instead
> of hardcode 0, 1, 0x21, "Genu" and etc.?
Thanks for reminder. It will be updated in the next version.
CPUID leaf 0x21 is newly added in [TDX] Section 10.2
TDX: https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
Can I add a definition of leaf 0x21 in MdePkg\Include\Register\Intel\Cpuid.h?
> 
> +
> +    mBaseXApicIsTdxEnabled = TRUE;
> 
> 4. avoid relying on global variable for caching the result.
Is it because LocalApicLib will run in flash?
> 
> +
> +  switch (MsrIndex) {
> +  case MSR_IA32_X2APIC_TPR:
> +  case MSR_IA32_X2APIC_PPR:
> +  case MSR_IA32_X2APIC_EOI:
> +  case MSR_IA32_X2APIC_ISR0:
> +  case MSR_IA32_X2APIC_ISR1:
> +  case MSR_IA32_X2APIC_ISR2:
> +  case MSR_IA32_X2APIC_ISR3:
> +  case MSR_IA32_X2APIC_ISR4:
> +  case MSR_IA32_X2APIC_ISR5:
> +  case MSR_IA32_X2APIC_ISR6:
> +  case MSR_IA32_X2APIC_ISR7:
> +  case MSR_IA32_X2APIC_TMR0:
> +  case MSR_IA32_X2APIC_TMR1:
> +  case MSR_IA32_X2APIC_TMR2:
> +  case MSR_IA32_X2APIC_TMR3:
> +  case MSR_IA32_X2APIC_TMR4:
> +  case MSR_IA32_X2APIC_TMR5:
> +  case MSR_IA32_X2APIC_TMR6:
> +  case MSR_IA32_X2APIC_TMR7:
> +  case MSR_IA32_X2APIC_IRR0:
> +  case MSR_IA32_X2APIC_IRR1:
> +  case MSR_IA32_X2APIC_IRR2:
> +  case MSR_IA32_X2APIC_IRR3:
> +  case MSR_IA32_X2APIC_IRR4:
> +  case MSR_IA32_X2APIC_IRR5:
> +  case MSR_IA32_X2APIC_IRR6:
> +  case MSR_IA32_X2APIC_IRR7:
> 
> 5. Can you explain in the comments about  what spec says that above MSR can
> be accessed directly while others cannot?
TDX: https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
[TDX] Section 18.1
> 
> 
> +  UINT64    Val;
> +  UINT64    Status;
> +  if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
> 
> 6. can we simplify the above check with " if (!AccessMsrNative (MsrIndex))"?
> IsTdxGuest() can be called inside AccessMsrNative().
Sure. It will be updated in next version.
> 
> +UINT32
> +EFIAPI
> 
> 7. No EFIAPI please.
Sure. It will be fixed in next version.
> 
> +ReadMsrReg32 (
> +  IN UINT32 MsrIndex
> +  )
> +{
> +  UINT64    Val;
> +  UINT64    Status;
> +  if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
> +    Status = TdVmCall (TDVMCALL_RDMSR, (UINT64) MsrIndex, 0, 0, 0, &Val);
> +    if (Status != 0) {
> +      TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0);
> +    }
> +  } else {
> +    Val = AsmReadMsr32 (MsrIndex);
> +  }
> +  return (UINT32)(UINTN) Val;
> 
> 8. Can you directly call ReadMsrReg64()?
Sure. It will be updated in next version.
> 
> 
> +VOID
> +EFIAPI
> +WriteMsrReg32 (
> +  IN UINT32 MsrIndex,
> +  IN UINT32 Val
> +  )
> +{
> +  UINT64    Status;
> +  if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
> +    Status = TdVmCall (TDVMCALL_WRMSR, (UINT64) MsrIndex, (UINT64) Val, 0,
> 0, 0);
> +    if (Status != 0) {
> +      DEBUG((DEBUG_ERROR, "WriteMsrReg32 returned failure.
> Status=0x%llx\n", Status));
> +      TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0);
> +    }
> +  } else {
> +    AsmWriteMsr32 (MsrIndex, Val);
> 
> 8. Can you directly call WriteMsrReg64()?
Sure. It will be updated in next version.
> 
> 
> -  ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
> +  ApicBaseMsr.Uint64 = ReadMsrReg64 (MSR_IA32_APIC_BASE);
> 
> 9. I prefer to use "LocalApicLibReadMsr64()". It indicates two meanings:
>     a. it's a local function which can be found within this lib
>     b. it's consistent with "AsmReadMsr64".
Sure. It will be updated in next version.
> 

Thanks.
Min