From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f177.google.com (mail-yb1-f177.google.com [209.85.219.177]) by mx.groups.io with SMTP id smtpd.web09.10443.1636540696174867124 for ; Wed, 10 Nov 2021 02:38:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@google.com header.s=20210112 header.b=fyEf4e+Q; spf=pass (domain: google.com, ip: 209.85.219.177, mailfrom: erdemaktas@google.com) Received: by mail-yb1-f177.google.com with SMTP id d10so5291939ybe.3 for ; Wed, 10 Nov 2021 02:38:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uaZcugo77orYzxCtr4sJAfJ8bhzmBDp4LjyRjDsz1yY=; b=fyEf4e+Q6vcR0VkHRLvtPMwBWx0giTaxshI6qDQPHtFx1lppeRE2/oHwGnqGfL9mqS 2gAH5zuWCK3zuHHiDInhJTegoFLpZaY3obQTbo1NMpJ13ZiI3DNVRTCMVs6tw1qTr8Si bbMxzLOl6/X/zPKHeJG3xrvK0IQcKORJYKLmxuEQmbLyrFaXw2irZR2sF64kTuFKDmA2 yAA+6F0RWCjkYR67yk1YX2aAMmYsotZ3lFyoZF65ikAECU9crmdcmHc8hrnCmVv/zzBl tDWI0AzleOHMFi6CEHe4oj5yLwtwJRg3E2Aj2mHTV619tCLVzVZEsuMo66Z+xda45yw4 E/yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uaZcugo77orYzxCtr4sJAfJ8bhzmBDp4LjyRjDsz1yY=; b=NQXzsR3YqG54rse5VnSGIGB/4JUaU3ZZHTjzfRvWvdhGOutWfXFV67v6EQeRcx8Dsx RpXWNJYKnAShhihOc5f4huHSUjyItZinY/a/l62OP2ZfDYhAhze2VcDBUDq9k+AZVKyC 2kA10y+aaMp3+tt/4XkfEF2p/R0CfsvQLHrq6Cf09UxEphhVvQeuLtWxLKtEsH7jiXxL 1RZXNwiSO6Yto4fwIxs4GBqzjnPQIjXfjH+ZQchHjVQmr7+NAFwOPVT0X07la2rMkh7t dPXJDPkOtxAr4ypBlE8PndP5aBVp2/UJkPXU51anoQsu0azNVYbztq+dJ1fTscmoc+TH YK5w== X-Gm-Message-State: AOAM533vhJUFI+IVEeVIe9HiuqhG7ytdJ/L3NfuN/xCuBGYotLWpRgHj bRagmGD6eDnmx3gJ3A0VZkkEyQKWinej8wp2eyHGZQ== X-Google-Smtp-Source: ABdhPJwj1NFPWOwoMxGPTkCxNYGylIU0Co8BrAYWc+ce8g0slrOvp377qpUBjD+YLY8SCjWOHgQ9jNFpIus2m+mUg2M= X-Received: by 2002:a5b:10a:: with SMTP id 10mr15186497ybx.535.1636540694963; Wed, 10 Nov 2021 02:38:14 -0800 (PST) MIME-Version: 1.0 References: <232162e8ee92888f7ac9795ee9baa331ea39c613.1635769996.git.min.m.xu@intel.com> In-Reply-To: <232162e8ee92888f7ac9795ee9baa331ea39c613.1635769996.git.min.m.xu@intel.com> From: "Erdem Aktas" Date: Wed, 10 Nov 2021 02:38:03 -0800 Message-ID: Subject: Re: [PATCH V3 02/29] MdePkg: Add TdxLib to wrap Tdx operations To: Min Xu Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu , Brijesh Singh , James Bottomley , Jiewen Yao , Tom Lendacky , Gerd Hoffmann Content-Type: text/plain; charset="UTF-8" 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 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 > +