On Sat, 2022-10-29 at 23:22 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> >
> > 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?
