On Thu, Nov 10, 2022 at 11:04:25AM +0000,
"Huang, Kai" <kai.huang@xxxxxxxxx> 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@xxxxxxxxx>
