On Thu, Jul 28, 2022 at 12:41:51PM -0700,
David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index f4d4ed41641b..bfc934dc9a33 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -716,6 +716,7 @@ struct kvm_vcpu_arch {
> > struct kvm_mmu_memory_cache mmu_shadow_page_cache;
> > struct kvm_mmu_memory_cache mmu_gfn_array_cache;
> > struct kvm_mmu_memory_cache mmu_page_header_cache;
> > + struct kvm_mmu_memory_cache mmu_private_sp_cache;
>
> I notice that mmu_private_sp_cache.gfp_zero is left unset so these pages
> may contain garbage. Is this by design because the TDX module can't rely
> on the contents being zero and has to take care of initializing the page
> itself? i.e. GFP_ZERO would be a waste of cycles?
>
> If I'm correct please include a comment here in the next revision to
> explain why GFP_ZERO is not necessary.
Yes, exactly. Here is the added comments.
/*
* This cache is to allocate pages used for Secure-EPT used by the TDX
* module. Because the TDX module doesn't trust VMM and initializes the
* pages itself, KVM doesn't initialize them. Allocate pages with
* garbage and give them to the TDX module.
*/
> > /*
> > * QEMU userspace and the guest each have their own FPU state.
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index c517c7bca105..a5bf3e40e209 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -691,6 +691,13 @@ static int mmu_topup_shadow_page_cache(struct kvm_vcpu *vcpu)
> > int start, end, i, r;
> > bool is_tdp_mmu = is_tdp_mmu_enabled(vcpu->kvm);
> >
> > + if (kvm_gfn_shared_mask(vcpu->kvm)) {
> > + r = kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_private_sp_cache,
> > + PT64_ROOT_MAX_LEVEL);
> > + if (r)
> > + return r;
> > + }
> > +
> > if (is_tdp_mmu && shadow_nonpresent_value)
> > start = kvm_mmu_memory_cache_nr_free_objects(mc);
> >
> > @@ -732,6 +739,7 @@ static void mmu_free_memory_caches(struct kvm_vcpu *vcpu)
> > {
> > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_pte_list_desc_cache);
> > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_shadow_page_cache);
> > + kvm_mmu_free_memory_cache(&vcpu->arch.mmu_private_sp_cache);
> > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_gfn_array_cache);
> > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_page_header_cache);
> > }
> > @@ -1736,6 +1744,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct
> > if (!direct)
> > sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache);
> > set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
> > + kvm_mmu_init_private_sp(sp, NULL);
>
> This is unnecessary. kvm_mmu_page structs are zero-initialized so
> private_sp will already be NULL.
Ok.
> >
> > /*
> > * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages()
> > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
> > index 44a04fad4bed..9f3a6bea60a3 100644
> > --- a/arch/x86/kvm/mmu/mmu_internal.h
> > +++ b/arch/x86/kvm/mmu/mmu_internal.h
> > @@ -55,6 +55,10 @@ struct kvm_mmu_page {
> > u64 *spt;
> > /* hold the gfn of each spte inside spt */
> > gfn_t *gfns;
> > +#ifdef CONFIG_KVM_MMU_PRIVATE
> > + /* associated private shadow page, e.g. SEPT page. */
>
> Can we use "Secure EPT" instead of SEPT in KVM code and comments? (i.e.
> also including variable names like sept_page -> secure_ept_page)
>
> "SEPT" looks like a mispelling of SPTE, which is used all over KVM. It
> will be difficult to read code that contains both acronyms.
Makes sense. Will update it.
> > + void *private_sp;
>
> Please name this "private_spt" and move it up next to "spt".
>
> sp" or "shadow page" is used to refer to kvm_mmu_page structs. For
> example, look at all the code in KVM that uses `struct kvm_mmu_page *sp`.
>
> "spt" is "shadow page table", i.e. the actual page table memory. See
> kvm_mmu_page.spt. Calling this field "private_spt" makes it obvious that
> this pointer is pointing to a page table.
>
> Also please update the language in the comment accordingly to "private
> shadow page table".
I'll rename as follows
private_sp => private_spt
spet_page => private_spt
mmu_private_sp_cache => mmu_private_spt_cache
kvm_mmu_init_private_sp => kvm_mmu_inite_private_spt
kvm_mmu_alloc_private_sp => kvm_mmu_alloc_private_spt
kvm_mmu_free_private_sp => kvm_mmu_free_private_spt
kvm_alloc_private_sp_for_split => kvm_alloc_private_spt_for_split
Thanks,
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
