Min,
Comments below:
-----Original Message-----
From: Xu, Min M <min.m...@intel.com>
Sent: Tuesday, October 5, 2021 11:39 AM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m...@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray
<ray...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Brijesh Singh
<brijesh.si...@amd.com>; Erdem Aktas <erdemak...@google.com>; James Bottomley
<j...@linux.ibm.com>; Yao, Jiewen <jiewen....@intel.com>; Tom Lendacky
<thomas.lenda...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
