RFC： https://bugzilla.tianocore.org/show_bug.cgi?id=3429
MSR is accessed in BaseXApicX2ApicLib. In TDX some MSRs are accessed
directly from/to CPU. Some should be accessed via explicit requests
from the host VMM using TDCALL(TDG.VP.VMCALL). This is done by the
help of TdxLib.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
.../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 233 +++++++++++++++++-
.../BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf | 1 +
UefiCpuPkg/UefiCpuPkg.dsc | 1 +
3 files changed, 227 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index cdcbca046191..eaa132ea30f4 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -23,11 +23,227 @@
#include <Library/TimerLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiCpuLib.h>
+#include <IndustryStandard/Tdx.h>
+#include <Library/TdxLib.h>
//
// Library internal functions
//
+BOOLEAN mBaseXApicIsTdxEnabled = FALSE;
+BOOLEAN mBaseXApicTdxProbed = FALSE;
+
+/**
+ Check if it is Tdx guest.
+
+ @return TRUE It is Tdx guest
+ @return FALSE It is not Tdx guest
+
+**/
+BOOLEAN
+EFIAPI
+BaseXApicIsTdxGuest (
+ VOID
+ )
+{
+ UINT32 Eax;
+ UINT32 Ebx;
+ UINT32 Ecx;
+ UINT32 Edx;
+ UINT32 LargestEax;
+
+ if (mBaseXApicTdxProbed) {
+ return mBaseXApicIsTdxEnabled;
+ }
+
+ mBaseXApicIsTdxEnabled = FALSE;
+
+ 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;
+ }
+
+ mBaseXApicIsTdxEnabled = TRUE;
+ }while (FALSE);
+
+ mBaseXApicTdxProbed = TRUE;
+
+ return mBaseXApicIsTdxEnabled;
+}
+
+
+/**
+ Some MSRs in TDX are directly read/write from/to CPU.
+
+ @param MsrIndex Index of the MSR
+ @retval TRUE MSR direct read/write from/to CPU.
+ @retval FALSE MSR not direct read/write from/to CPU.
+
+**/
+BOOLEAN
+EFIAPI
+AccessMsrNative (
+ IN UINT32 MsrIndex
+ )
+{
+ 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:
+ return TRUE;
+ default:
+ break;
+ }
+ return FALSE;
+}
+
+/**
+ Read MSR value.
+
+ @param MsrIndex Index of the MSR to read
+ @retval 64-bit Value of MSR.
+
+**/
+UINT64
+EFIAPI
+ReadMsrReg64 (
+ 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 = AsmReadMsr64 (MsrIndex);
+ }
+ return Val;
+}
+
+/**
+ Write to MSR.
+
+ @param MsrIndex Index of the MSR to write to
+ @param Val Value to be written to the MSR
+
+**/
+VOID
+EFIAPI
+WriteMsrReg64 (
+ IN UINT32 MsrIndex,
+ IN UINT64 Val
+ )
+{
+ UINT64 Status;
+ if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
+ Status = TdVmCall (TDVMCALL_WRMSR, (UINT64) MsrIndex, Val, 0, 0, 0);
+ if (Status != 0) {
+ TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0);
+ }
+ } else {
+ AsmWriteMsr64 (MsrIndex, Val);
+ }
+}
+
+/**
+ Read MSR value.
+
+ @param MsrIndex Index of the MSR to read
+ @retval 32-bit Value of MSR.
+
+**/
+UINT32
+EFIAPI
+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;
+}
+
+/**
+ Write to MSR.
+
+ @param MsrIndex Index of the MSR to write to
+ @param Val Value to be written to the MSR
+
+**/
+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);
+ }
+}
+
/**
Determine if the CPU supports the Local APIC Base Address MSR.
@@ -77,7 +293,7 @@ GetLocalApicBaseAddress (
return PcdGet32 (PcdCpuLocalApicBaseAddress);
}
- ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
+ ApicBaseMsr.Uint64 = ReadMsrReg64 (MSR_IA32_APIC_BASE);
return (UINTN)(LShiftU64 ((UINT64) ApicBaseMsr.Bits.ApicBaseHi, 32)) +
(((UINTN)ApicBaseMsr.Bits.ApicBase) << 12);
@@ -108,12 +324,12 @@ SetLocalApicBaseAddress (
return;
}
- ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
+ ApicBaseMsr.Uint64 = ReadMsrReg64 (MSR_IA32_APIC_BASE);
ApicBaseMsr.Bits.ApicBase = (UINT32) (BaseAddress >> 12);
ApicBaseMsr.Bits.ApicBaseHi = (UINT32) (RShiftU64((UINT64) BaseAddress, 32));
- AsmWriteMsr64 (MSR_IA32_APIC_BASE, ApicBaseMsr.Uint64);
+ WriteMsrReg64 (MSR_IA32_APIC_BASE, ApicBaseMsr.Uint64);
}
/**
@@ -153,7 +369,7 @@ ReadLocalApicReg (
ASSERT (MmioOffset != XAPIC_ICR_HIGH_OFFSET);
MsrIndex = (UINT32)(MmioOffset >> 4) + X2APIC_MSR_BASE_ADDRESS;
- return AsmReadMsr32 (MsrIndex);
+ return ReadMsrReg32 (MsrIndex);
}
}
@@ -202,7 +418,7 @@ WriteLocalApicReg (
// Use memory fence here to force the serializing semantics to be consisent with xAPIC mode.
//
MemoryFence ();
- AsmWriteMsr32 (MsrIndex, Value);
+ WriteMsrReg32 (MsrIndex, Value);
}
}
@@ -309,7 +525,7 @@ GetApicMode (
return LOCAL_APIC_MODE_XAPIC;
}
- ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
+ ApicBaseMsr.Uint64 = ReadMsrReg64 (MSR_IA32_APIC_BASE);
//
// Local APIC should have been enabled
//
@@ -350,13 +566,14 @@ SetApicMode (
CurrentMode = GetApicMode ();
if (CurrentMode == LOCAL_APIC_MODE_XAPIC) {
+
switch (ApicMode) {
case LOCAL_APIC_MODE_XAPIC:
break;
case LOCAL_APIC_MODE_X2APIC:
- ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
+ ApicBaseMsr.Uint64 = ReadMsrReg64 (MSR_IA32_APIC_BASE);
ApicBaseMsr.Bits.EXTD = 1;
- AsmWriteMsr64 (MSR_IA32_APIC_BASE, ApicBaseMsr.Uint64);
+ WriteMsrReg64 (MSR_IA32_APIC_BASE, ApicBaseMsr.Uint64);
break;
default:
ASSERT (FALSE);
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
index 1e2a4f8b790f..1276f6ec06d6 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
@@ -39,6 +39,7 @@
IoLib
PcdLib
UefiCpuLib
+ TdxLib
[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 870b45284087..e5e6bf77c8e2 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -61,6 +61,7 @@
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+ TdxLib|MdePkg/Library/TdxLib/TdxLib.inf
[LibraryClasses.common.SEC]
PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81479): https://edk2.groups.io/g/devel/message/81479
Mute This Topic: https://groups.io/mt/86085736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi,
> + do {
> + AsmCpuid (0, &LargestEax, &Ebx, &Ecx, &Edx);
Use ConfidentialComputing PCD ?
> +BOOLEAN
> +EFIAPI
> +AccessMsrNative (
I'd suggest to reverse the logic, i.e. have a AccessMsrTdxCall() which
returns true in case (a) tdx is active and (b) the msr is not on the
white list for native access ...
> +{
> + UINT64 Val;
> + UINT64 Status;
> + if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
... the just use "if (AccessMsrTdxCall(MsrIndex)) { ..." here.
Beside that: Are the apic msr registers the only ones which can
be accessed directly?
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81819): https://edk2.groups.io/g/devel/message/81819
Mute This Topic: https://groups.io/mt/86085736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On October 12, 2021 6:16 PM, Gerd Hoffman wrote:
> Hi,
>
> > + do {
> > + AsmCpuid (0, &LargestEax, &Ebx, &Ecx, &Edx);
>
> Use ConfidentialComputing PCD ?
BaseXApicX2ApicLib (LocalApicLib) is included by the drivers/libs not only in DXE phase, but also in SEC/PEI. For example, SecPeiCpuExceptionHandlerLib is included in SEC/PEI_CORE/PEIM. In SEC phase ConfidentialComputing PCD has not been set. So it cannot be used in SEC phase to determine if it is TDX guest or not.
That's why CPUID is used in BaseXApicX2ApicLib so that it works in SEC/PEI/DXE phases.
>
> > +BOOLEAN
> > +EFIAPI
> > +AccessMsrNative (
>
> I'd suggest to reverse the logic, i.e. have a AccessMsrTdxCall() which returns
> true in case (a) tdx is active and (b) the msr is not on the white list for native
> access ...
>
> > +{
> > + UINT64 Val;
> > + UINT64 Status;
> > + if (!AccessMsrNative (MsrIndex) && BaseXApicIsTdxGuest ()) {
>
> ... the just use "if (AccessMsrTdxCall(MsrIndex)) { ..." here.
>
Ok, It will be updated in the next version.
> Beside that: Are the apic msr registers the only ones which can be accessed
> directly?
TDX: https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
Section 10.7 MSR Handling
Section 18.1 Table 18.2 MSR Virtualization
X2APIC MSR Registers which can be accessed natively is in above table.
>
Thanks!
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81886): https://edk2.groups.io/g/devel/message/81886
Mute This Topic: https://groups.io/mt/86085736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Min,
Comments below:
-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Tuesday, October 5, 2021 11:39 AM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81983): https://edk2.groups.io/g/devel/message/81983
Mute This Topic: https://groups.io/mt/86085736/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.