Date:   Thu, 10 Nov 2022 11:04:25 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
        "dmatlack@...gle.com" <dmatlack@...gle.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v10 016/108] KVM: TDX: create/destroy VM structure

On Sat, 2022-10-29 at 23:22 -0700, isaku.yamahata@...el.com wrote:
> > From: Sean Christopherson <sean.j.christopherson@...el.com>
> > 
> > As the first step to create TDX guest, create/destroy VM struct.  Assign
> > TDX private Host Key ID (HKID) to the TDX guest for memory encryption and
> > allocate extra pages for the TDX guest. On destruction, free allocated
> > pages, and HKID.
> > 
> > Before tearing down private page tables, TDX requires some resources of the
> > guest TD to be destroyed (i.e. keyID must have been reclaimed, etc).  Add
				 ^
				 HKID

It's better to use consistent words throughout patch/series I guess.

> > flush_shadow_all_private callback before tearing down private page tables
> > for it.
> > 
> > Add a second kvm_x86_ops hook in kvm_arch_destroy_vm() to support TDX's
> > destruction path, which needs to first put the VM into a teardown state,
> > then free per-vCPU resources, and finally free per-VM resources.

Perhaps explicitly call out the hook is vm_free() and why the existing
vm_destroy() hook cannot meet TDX's purpose, so that people can understand
easily why you need vm_free().

[...]

> > +static inline void tdx_hkid_free(struct kvm_tdx *kvm_tdx)
> > +{
> > +	tdx_keyid_free(kvm_tdx->hkid);
> > +	kvm_tdx->hkid = -1;

Why -1? Can it be set to 0, which is the initial value when kvm_tdx is allocated
anyway?

> > +}
> > +
> > +static inline bool is_hkid_assigned(struct kvm_tdx *kvm_tdx)
> > +{
> > +	return kvm_tdx->hkid > 0;
> > +}
> > +
> > +static void tdx_clear_page(unsigned long page)
> > +{
> > +	const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
> > +	unsigned long i;
> > +
> > +	/*
> > +	 * Zeroing the page is only necessary for systems with MKTME-i:
> > +	 * when re-assign one page from old keyid to a new keyid, MOVDIR64B is
> > +	 * required to clear/write the page with new keyid to prevent integrity
> > +	 * error when read on the page with new keyid.
> > +	 *
> > +	 * The cache line could be poisoned (even without MKTME-i), clear the
> > +	 * poison bit.

Does this happen only when there's potential kernel bug?

> > +	 */
> > +	for (i = 0; i < PAGE_SIZE; i += 64)
> > +		movdir64b((void *)(page + i), zero_page);
> > +	/*
> > +	 * MOVDIR64B store uses WC buffer.  Prevent following memory reads
> > +	 * from seeing potentially poisoned cache.
> > +	 */
> > +	__mb();
> > +}
> > +
> > +static int tdx_reclaim_page(unsigned long va, hpa_t pa, bool do_wb, u16 hkid)
> > +{
> > +	struct tdx_module_output out;
> > +	u64 err;
> > +
> > +	do {
> > +		err = tdh_phymem_page_reclaim(pa, &out);
> > +		/*
> > +		 * TDH.PHYMEM.PAGE.RECLAIM is allowed only when TD is shutdown.
> > +		 * state.  i.e. destructing TD.
> > +		 * TDH.PHYMEM.PAGE.RECLAIM  requires TDR and target page.
> > +		 * Because we're destructing TD, it's rare to contend with TDR.
> > +		 */
> > +	} while (err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX));
> > +	if (WARN_ON_ONCE(err)) {
> > +		pr_tdx_error(TDH_PHYMEM_PAGE_RECLAIM, err, &out);
> > +		return -EIO;
> > +	}
> > +
> > +	if (do_wb) {
> > +		/*
> > +		 * Only TDR page gets into this path.  No contention is expected
> > +		 * because the last page of TD.
> > +		 */
> > +		err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(pa, hkid));
> > +		if (WARN_ON_ONCE(err)) {
> > +			pr_tdx_error(TDH_PHYMEM_PAGE_WBINVD, err, NULL);
> > +			return -EIO;
> > +		}
> > +	}
> > +
> > +	tdx_clear_page(va);
> > +	return 0;
> > +}
> > +
> > +static int tdx_alloc_td_page(struct tdx_td_page *page)
> > +{
> > +	page->va = __get_free_page(GFP_KERNEL_ACCOUNT);
> > +	if (!page->va)
> > +		return -ENOMEM;
> > +
> > +	page->pa = __pa(page->va);
> > +	return 0;
> > +}
> > +
> > +static inline void tdx_mark_td_page_added(struct tdx_td_page *page)
> > +{
> > +	WARN_ON_ONCE(page->added);
> > +	page->added = true;
> > +}
> > +
> > +static void tdx_reclaim_td_page(struct tdx_td_page *page)
> > +{
> > +	if (page->added) {
> > +		/*
> > +		 * TDCX are being reclaimed.  TDX module maps TDCX with HKID
> > +		 * assigned to the TD.  Here the cache associated to the TD
> > +		 * was already flushed by TDH.PHYMEM.CACHE.WB before here, So
> > +		 * cache doesn't need to be flushed again.
> > +		 */
> > +		if (tdx_reclaim_page(page->va, page->pa, false, 0))
> > +			return;
> > +
> > +		page->added = false;
> > +	}
> > +	if (page->va) {
> > +		free_page(page->va);
> > +		page->va = 0;
> > +	}
> > +}
> > +


I am wondering why this 'struct tdx_td_page' is needed?

It appears the page->pa is used by SEAMCALLs and page->va is used by
tdx_clear_page() as MOVDIR64B needs a virtual address.

But since GFP_KERNEL_ACCOUNT is used in memory allocation, so you can actually
just get the pa and va from the page easily (using page_to_phys() and __va()).
Also it's 64-bit kernel so you don't even need to consider HIGHMEM.

Also, it seems page->added can be replaced with simply checking whether page is
NULL, correct?

Btw, I think the introduce of 'struct tdx_td_page' and the new 'struct
tdx_td_page tdr' to 'struct kvm_tdx' should come together with this patch, but
not in the previous patch "KVM: TDX: Stub in tdx.h with structs, accessors, and
VMCS helpers". This makes the code review easier.

The "accessors" can be introduced in later patch when they are needed -- it
doesn't seem any of them is used in this patch?

Powered by blists - more mailing lists