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/DebugLib.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 (#83675): https://edk2.groups.io/g/devel/message/83675
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]
-=-=-=-=-=-=-=-=-=-=-=-
