From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.10647.1634026933459923650 for ; Tue, 12 Oct 2021 01:22:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Xm4YmAtz; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634026932; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=HT2R9XmX7D9R7daw/ZAPQJtgPIpJ6FNBXOFTh2M12KI=; b=Xm4YmAtzdizvTmQvvfXATPR3Vh5fbh+K0NigcbPVJ+XCn6mJfHzdiMH1CMHIWy2nKj4j5K 7o7x4W6th5sI+vkfeh0afVu2zr8QPZp2WeamtFYhStq7drvxp6l2zLrNBxRTUznOW95ZO0 ssBVME+NoFE7tzhhy+cQ/8c8oGe748k= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-411-DlOj8vLVMaaVDKZrU0vAFg-1; Tue, 12 Oct 2021 04:22:11 -0400 X-MC-Unique: DlOj8vLVMaaVDKZrU0vAFg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 73B2B800FF0; Tue, 12 Oct 2021 08:22:09 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.193.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E93FD57CA3; Tue, 12 Oct 2021 08:22:08 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id C26231800393; Tue, 12 Oct 2021 10:22:06 +0200 (CEST) Date: Tue, 12 Oct 2021 10:22:06 +0200 From: "Gerd Hoffmann" To: devel@edk2.groups.io, min.m.xu@intel.com Cc: Michael D Kinney , Liming Gao , Zhiguang Liu , Brijesh Singh , Erdem Aktas , James Bottomley , Jiewen Yao , Tom Lendacky Subject: Re: [edk2-devel] [PATCH V2 05/28] MdePkg: Add TdxLib to wrap Tdx operations Message-ID: <20211012082206.2j5eptadquhf3pmg@sirius.home.kraxel.org> References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=kraxel@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline > +// PageSize is mapped to PageLevel like below: > +// 4KB - 0, 2MB - 1 > +UINT64 mTdxAcceptPageLevelMap[2] = { > + SIZE_4KB, > + SIZE_2MB No 1G pages? > +UINTN > +GetGpaPageLevel ( > + UINT64 PageSize Uh, UINT32 is not enough? Keep the door open for 512G pages? > +{ > + UINTN Index; > + > + for (Index = 0; Index < sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0]); Index++) { There is an ARRAY_SIZE() macro, no need to open code the sizeof() trick. > + if (mTdxAcceptPageLevelMap[Index] == PageSize) { > + break; > + } > + } > + > + return Index; > +} No error handling (invalid PageSize) here? > +/** > + This function accept a pending private page, and initialize the page to > + all-0 using the TD ephemeral private key. > + > + Sometimes TDCALL [TDG.MEM.PAGE.ACCEPT] may return > + TDX_EXIT_REASON_PAGE_SIZE_MISMATCH. It indicates the input PageLevel is > + not workable. In this case we need to try to fallback to a smaller > + PageLevel if possible. > + > + @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. Only accept 1G/2M/4K size. > + > + @return EFI_SUCCESS Accept successfully > + @return others Indicate other errors > +**/ > +EFI_STATUS > +EFIAPI > +TdAcceptPages ( > + IN UINT64 StartAddress, > + IN UINT64 NumberOfPages, > + IN UINT64 PageSize > + ) > +{ > + EFI_STATUS Status; > + UINT64 Address; > + UINT64 TdxStatus; > + UINT64 Index; > + UINT64 GpaPageLevel; > + UINT64 PageSize2; > + > + Address = StartAddress; > + > + GpaPageLevel = (UINT64) GetGpaPageLevel (PageSize); Why cast? > + if (GpaPageLevel > sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0])) { > + DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size - 0x%llx\n", PageSize)); Ah. Errors are catched here. Well, no. The check is wrong, it should be ">=" not ">". Better would be GetGpaPageLevel explicitly returning a specific value (for example -1) on error. > + Status = EFI_SUCCESS; > + for (Index = 0; Index < NumberOfPages; Index++) { > + TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0, 0); > + if (TdxStatus != TDX_EXIT_REASON_SUCCESS) { > + if ((TdxStatus & ~0xFFFFULL) == TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) { > + // > + // Already accepted > + // > + mNumberOfDuplicatedAcceptedPages++; Hmm. When this happens we have a bug somewhere, right? So should this be an assert()? Or should we at least log the address? > +#define RTMR_COUNT 4 > +#define TD_EXTEND_BUFFER_LEN (64 + 64) > +#define EXTEND_BUFFER_ADDRESS_MASK 0x3f > + > + > +#pragma pack(16) > +typedef struct { > + UINT8 Buffer[TD_EXTEND_BUFFER_LEN]; > +} TDX_EXTEND_BUFFER; > +#pragma pack() > + > +UINT8 *mExtendBufferAddress = NULL; > +TDX_EXTEND_BUFFER mExtendBuffer; > + > +/** > + TD.RTMR.EXTEND requires 64B-aligned guest physical address of > + 48B-extension data. In runtime we walk thru the Buffer to find > + out a 64B-aligned start address. Can't you just use __attribute__((aligned(64))) for that? take care, Gerd