Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Nov 2022 09:45:08 -0800
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Shahar, Sagi" <sagis@google.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
        "dmatlack@google.com" <dmatlack@google.com>,
        "Christopherson,, Sean" <seanjc@google.com>
Subject: Re: [PATCH v10 016/108] KVM: TDX: create/destroy VM structure
Message-ID: <20221117174508.GD2350331@ls.amr.corp.intel.com>
References: <cover.1667110240.git.isaku.yamahata@intel.com>
 <fb337a67e17715977e46523d1344cb2a7f46a37a.1667110240.git.isaku.yamahata@intel.com>
 <c3deef412933070932f565af8639d15aef00ea1c.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c3deef412933070932f565af8639d15aef00ea1c.camel@intel.com>
Precedence: bulk

On Thu, Nov 10, 2022 at 11:04:25AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> > > 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().

Sure, will update the commit message.


> > > +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?

0 works. I'll replace it with 0.


> > > +}
> > > +
> > > +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?

That's right.


> > > +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.

Yes, page->va member can be dropped. Will drop it.


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

No. It's subtle. Anyway let me check it.


> 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?

Ok, will move it to that patch.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>