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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
