BTW: Is this internal API? I feel we should add ASSERT() for invalid page size as well, to catch issue earlier.
Thank you > -----Original Message----- > From: Xu, Min M <min.m...@intel.com> > Sent: Friday, November 12, 2021 10:39 AM > To: Erdem Aktas <erdemak...@google.com> > Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>; > Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang > <zhiguang....@intel.com>; Brijesh Singh <brijesh.si...@amd.com>; James > Bottomley <j...@linux.ibm.com>; Yao, Jiewen <jiewen....@intel.com>; Tom > Lendacky <thomas.lenda...@amd.com>; Gerd Hoffmann <kra...@redhat.com> > Subject: RE: [PATCH V3 02/29] MdePkg: Add TdxLib to wrap Tdx operations > > On November 10, 2021 6:38 PM, Erdem Aktas wrote: > > 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. > Ah, -1 should not be returned because the function returns unsigned integer. > It will be fixed in the next version like this: > > UINT32 mTdxAcceptPageLevelMap[2] = { > SIZE_4KB, > SIZE_2MB > }; > #define INVALID_ACCEPT_PAGELEVEL ARRAY_SIZE(mTdxAcceptPageLevelMap) > > UINT32 > GetGpaPageLevel ( > UINT32 PageSize > ) > { > UINT32 Index; > for (Index = 0; Index < ARRAY_SIZE (mTdxAcceptPageLevelMap); Index++) { > if (mTdxAcceptPageLevelMap[Index] == PageSize) { > break; > } > } > return Index; > } > ... ... > GpaPageLevel = GetGpaPageLevel (PageSize); > if (GpaPageLevel == INVALID_ACCEPT_PAGELEVEL) { > DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size - > 0x%llx\n", PageSize)); > return EFI_INVALID_PARAMETER; > } > > > > +/** > > + 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. > Currently 2M/4K accept page size is supported. The comments will be fixed in > the next version. > > > > + > > + @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? > It will be fixed. See my first comments. > > > > > + 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? > Right. The input address should be checked with Address[11:3] and [63:52]. > Address[2:0] should be 0 too. Because Address[2:0] will be set with Accept > Page > Level. > It will be fixed in the next version. > > > > > + 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. > Ideally a page should be accepted only once. But if a page is accepted twice, > it is > not an error (from the perspective of TdCall_Accept). In the PoC we do ran > into > such cases (it is a bug in the code). So we keep it as debug printing. > > > > > + 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? > Guest want to accept page with 2M size, but in some case, the VMM cannot do > that with 2M page size. In this case, Guest will get a returned result > (TDX_EXIT_REASON_PAGE_SIZE_MISMATCH) from the TdCall. So Guest falls > back to accept the page with a smaller page size. Currently only 2M/4K accept > page size is supported. In the future, 1G accept page size will be supported. > In > that case, there may be 2 fall backs: 1G->2M and 2M->4K. But if the accept > page > size is 4K, it cannot fall back to a smaller size. > Guest start to accept pages, VMM just response to the accept page command > from Guest. > > > > > + 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. > Thanks for reminder. It will be fixed in the next version. > > > > > + 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? > ASSERT () is for debugging purpose. It will be ignored in release mode. So the > following condition will run in release mode. > See > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/Debu > gLib.h#L385-L409 > > > > > + 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? > I don't think we should panic here. > Because TdVmCall is a common function (see sub-leaf of TDVMCALL). There are > various errors. Some of them are not so serious that panic is needed. Caller > of > TdVmCall will handle the returned result. > > > > > + 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. > Thanks for reminder. It will be updated in the next version. > > > > > +; 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. > Thanks for reminder. It will be fixed in the next version. > > > > > + 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 (#83676): https://edk2.groups.io/g/devel/message/83676 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] -=-=-=-=-=-=-=-=-=-=-=-
