[edk2-devel] [PATCH V2 07/28] UefiCpuPkg: Support TDX in BaseXApicX2ApicLib

[Ni, Ray]

Min,
Comments below:

-----Original Message-----
From: Xu, Min M <min.m.xu at intel.com> 
Sent: Tuesday, October 5, 2021 11:39 AM
To: devel at edk2.groups.io
Cc: Xu, Min M <min.m.xu at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Brijesh Singh <brijesh.singh at amd.com>; Erdem Aktas <erdemaktas at google.com>; James Bottomley <jejb at linux.ibm.com>; Yao, Jiewen <jiewen.yao at intel.com>; Tom Lendacky <thomas.lendacky at amd.com>
Subject: [PATCH V2 07/28] UefiCpuPkg: Support TDX in BaseXApicX2ApicLib

+**/
+BOOLEAN
+EFIAPI

1. EFIAPI is for public lib API. Is this a public API?

+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.


+
+  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.?

+
+    mBaseXApicIsTdxEnabled = TRUE;

4. avoid relying on global variable for caching the result.

+
+  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?


+  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().

+UINT32
+EFIAPI

7. No EFIAPI please.

+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()?


+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()?

 
-  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".




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81855): https://edk2.groups.io/g/devel/message/81855
Mute This Topic: https://groups.io/mt/86085736/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-