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