[edk2-devel] [PATCH V3 02/29] MdePkg: Add TdxLib to wrap Tdx operations
Erdem Aktas via groups.io
erdemaktas=google.com at groups.io
Wed Nov 10 10:38:03 UTC 2021
Hi Min,
Sorry for the late review. My comments and a few questions are inline.
Thanks
-Erdem
On Mon, Nov 1, 2021 at 6:16 AM Min Xu <min.m.xu at intel.com> wrote:
>
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>
....
> +**/
> +UINT32
> +GetGpaPageLevel (
> + UINT32 PageSize
> + )
> +{
> + UINT32 Index;
> +
> + for (Index = 0; Index < ARRAY_SIZE (mTdxAcceptPageLevelMap); Index++) {
> + if (mTdxAcceptPageLevelMap[Index] == PageSize) {
> + break;
> + }
> + }
> +
> + return Index == ARRAY_SIZE (mTdxAcceptPageLevelMap) ? -1 : Index;
Is this intentional? Returning signed integer but the function returns
unsigned integer.
+/**
+ This function accepts a pending private page, and initialize the page to
+ all-0 using the TD ephemeral private key.
+
+ @param[in] StartAddress Guest physical address of the private page
+ to accept.
+ @param[in] NumberOfPages Number of the pages to be accepted.
+ @param[in] PageSize GPA page size. Accept 1G/2M/4K page size.
The comment says that 1G is acceptable but the code only accepts 2M or
4K page sizes.
+
+ @return EFI_SUCCESS
> +EFI_STATUS
> +EFIAPI
> +TdAcceptPages (
> + IN UINT64 StartAddress,
> + IN UINT64 NumberOfPages,
> + IN UINT32 PageSize
> + )
> +{
> + EFI_STATUS Status;
> + UINT64 Address;
> + UINT64 TdxStatus;
> + UINT64 Index;
> + UINT32 GpaPageLevel;
> + UINT32 PageSize2;
> +
> + Address = StartAddress;
> +
> + GpaPageLevel = GetGpaPageLevel (PageSize);
> + if (GpaPageLevel == -1) {
Comparing unsigned int to signed int. I believe this should work with
GCC with warning messages.
Just checking if this is intentional?
> + DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size - 0x%llx\n", PageSize));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + Status = EFI_SUCCESS;
> + for (Index = 0; Index < NumberOfPages; Index++) {
> + TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0, 0);
Address[11:3] and [63:52] are reserved and must be 0. The code does
not check it, spec is not clear about the behavior but I am assuming
that in that case, TDX_OPERAND_INVALID will be returned as error code
but should we check and log it properly?
> + if (TdxStatus != TDX_EXIT_REASON_SUCCESS) {
> + if ((TdxStatus & ~0xFFFFULL) == TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) {
> + //
> + // Already accepted
> + //
> + mNumberOfDuplicatedAcceptedPages++;
Is there any legit case that a page should be accepted twice? Looks
like other than debug printing, this information is ignored.
> + DEBUG ((DEBUG_WARN, "Page at Address (0x%llx) has already been accepted. - %d\n", Address, mNumberOfDuplicatedAcceptedPages));
> + } else if ((TdxStatus & ~0xFFFFULL) == TDX_EXIT_REASON_PAGE_SIZE_MISMATCH) {
> + //
> + // GpaPageLevel is mismatch, fall back to a smaller GpaPageLevel if possible
> + //
> + DEBUG ((DEBUG_VERBOSE, "Address %llx cannot be accepted in PageLevel of %d\n", Address, GpaPageLevel));
> +
> + if (GpaPageLevel == 0) {
> + //
> + // Cannot fall back to smaller page level
> + //
Could you help me understand this? So if ,for some reason, VMM maps a
2MB private page and a guest wants to accept it in 4KB page chunks,
this will always fail. Is it not a possible use case?
> + DEBUG ((DEBUG_ERROR, "AcceptPage cannot fallback from PageLevel %d\n", GpaPageLevel));
> + Status = EFI_INVALID_PARAMETER;
> + break;
> + } else {
> + //
> + // Fall back to a smaller page size
> + //
> + PageSize2 = mTdxAcceptPageLevelMap [GpaPageLevel - 1];
> + Status = TdAcceptPages(Address, 512, PageSize2);
> + if (EFI_ERROR (Status)) {
> + break;
> + }
> + }
> + }else {
> +
> + //
> + // Other errors
> + //
Why not handle the TDX_OPERAND_BUSY? It is not an error and tdaccept
needs to be retired, I guess, handling it like an error might cause
reliability issues.
> + DEBUG ((DEBUG_ERROR, "Address %llx (%d) failed to be accepted. Error = 0x%llx\n",
> + Address, Index, TdxStatus));
> + Status = EFI_INVALID_PARAMETER;
> + break;
> + }
> + }
> + Address += PageSize;
> + }
> + return Status;
> +}
.....
> +**/
> +EFI_STATUS
> +EFIAPI
> +TdExtendRtmr (
> + IN UINT32 *Data,
> + IN UINT32 DataLen,
> + IN UINT8 Index
> + )
> +{
> + EFI_STATUS Status;
> + UINT64 TdCallStatus;
> + UINT8 *ExtendBuffer;
> +
> + Status = EFI_SUCCESS;
> +
> + ASSERT (Data != NULL);
> + ASSERT (DataLen == SHA384_DIGEST_SIZE);
> + ASSERT (Index >= 0 && Index < RTMR_COUNT);
> +
Because of the Asserts above , the following condition will never run, right?
> + if (Data == NULL || DataLen != SHA384_DIGEST_SIZE || Index >= RTMR_COUNT) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
...
> +; UINT64
> +; EFIAPI
> +; TdVmCall (
> +; UINT64 Leaf, // Rcx
> +; UINT64 P1, // Rdx
> +; UINT64 P2, // R8
> +; UINT64 P3, // R9
> +; UINT64 P4, // rsp + 0x28
> +; UINT64 *Val // rsp + 0x30
> +; )
> +global ASM_PFX(TdVmCall)
> +ASM_PFX(TdVmCall):
> + tdcall_push_regs
> +
> + mov r11, rcx
> + mov r12, rdx
> + mov r13, r8
> + mov r14, r9
> + mov r15, [rsp + first_variable_on_stack_offset ]
> +
> + tdcall_regs_preamble TDVMCALL, TDVMCALL_EXPOSE_REGS_MASK
> +
> + tdcall
> +
> + ; ignore return dataif TDCALL reports failure.
should we not panic here also?
> + test rax, rax
> + jnz .no_return_data
> +
> + ; Propagate TDVMCALL success/failure to return value.
> + mov rax, r10
> +
> + ; Retrieve the Val pointer.
> + mov r9, [rsp + second_variable_on_stack_offset ]
> + test r9, r9
> + jz .no_return_data
> +
> + ; On success, propagate TDVMCALL output value to output param
> + test rax, rax
> + jnz .no_return_data
> + mov [r9], r11
> +.no_return_data:
> + tdcall_regs_postamble
> +
> + tdcall_pop_regs
> +
> + ret
> +
> +;------------------------------------------------------------------------------
> +; 0 => RAX = TDCALL leaf
> +; M => RCX = TDVMCALL register behavior
> +; 1 => R10 = standard vs. vendor
> +; RDI => R11 = TDVMCALL function / nr
> +; RSI = R12 = p1
> +; RDX => R13 = p2
> +; RCX => R14 = p3
> +; R8 => R15 = p4
> +
Above comment does not match the below definition.
> +; UINT64
> +; EFIAPI
> +; TdVmCallCpuid (
> +; UINT64 EaxIn, // Rcx
> +; UINT64 EcxIn, // Rdx
> +; UINT64 *Results // R8
> +; )
> +global ASM_PFX(TdVmCallCpuid)
> +ASM_PFX(TdVmCallCpuid):
> + tdcall_push_regs
> +
> + mov r11, EXIT_REASON_CPUID
> + mov r12, rcx
> + mov r13, rdx
> +
> + ; Save *results pointers
> + push r8
> +
Looks like we are leaking r14 and r15 here.
> + tdcall_regs_preamble TDVMCALL, TDVMCALL_EXPOSE_REGS_MASK
> +
> + tdcall
> +
> + ; Panic if TDCALL reports failure.
> + test rax, rax
> + jnz .no_return_data
> +
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83543): https://edk2.groups.io/g/devel/message/83543
Mute This Topic: https://groups.io/mt/86739959/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list