[edk2-devel] [PATCH V3 02/29] MdePkg: Add TdxLib to wrap Tdx operations
Yao, Jiewen
jiewen.yao at intel.com
Fri Nov 12 02:42:11 UTC 2021
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.xu at intel.com>
> Sent: Friday, November 12, 2021 10:39 AM
> To: Erdem Aktas <erdemaktas at google.com>
> Cc: devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>;
> Liming Gao <gaoliming at byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu at intel.com>; Brijesh Singh <brijesh.singh at amd.com>; James
> Bottomley <jejb at linux.ibm.com>; Yao, Jiewen <jiewen.yao at intel.com>; Tom
> Lendacky <Thomas.Lendacky at amd.com>; Gerd Hoffmann <kraxel at 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.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.
> 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/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