On Thu, Jun 30, 2022 at 11:03:56PM +1200,
Kai Huang <kai.huang@xxxxxxxxx> wrote:
> On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> >
> > TDX introduced a new ETP, Secure-EPT, in addition to the existing EPT.
> > Secure-EPT maps protected guest memory, which is called private. Since
> > Secure-EPT page tables is also protected, those page tables is also called
> > private. The existing EPT is often called shared EPT to distinguish from
> > Secure-EPT. And also page tables for share EPT is also called shared.
>
> Does this patch has anything to do with secure-EPT?
>
> >
> > Virtualization Exception, #VE, is a new processor exception in VMX non-root
>
> #VE isn't new. It's already in pre-TDX public spec AFAICT.
>
> > operation. In certain virtualizatoin-related conditions, #VE is injected
> > into guest instead of exiting from guest to VMM so that guest is given a
> > chance to inspect it. One important one is EPT violation. When
> > "ETP-violation #VE" VM-execution is set, "#VE suppress bit" in EPT entry
> > is cleared, #VE is injected instead of EPT violation.
>
> We already know such fact based on pre-TDX public spec. Instead of repeating it
> here, why not focusing on saying what's new in TDX, so your below paragraph of
> setting a non-zero value for non-present SPTE can be justified?
Ok, will drop those two paragraph above.
> > Because guest memory is protected with TDX, VMM can't parse instructions
> > in the guest memory. Instead, MMIO hypercall is used for guest to pass
> > necessary information to VMM.
> >
> > To make unmodified device driver work, guest TD expects #VE on accessing
> > shared GPA. The #VE handler converts MMIO access into MMIO hypercall with
> > the EPT entry of enabled "#VE" by clearing "suppress #VE" bit. Before VMM
> > enabling #VE, it needs to figure out the given GPA is for MMIO by EPT
> > violation.
> >
>
> As I said above, before here, you need to explain in TDX VMCS is controlled by
> the TDX module and it always sets the "EPT-violation #VE" in execution control
> bit.
>
> > So the execution flow looks like
> >
> > - Allocate unused shared EPT entry with suppress #VE bit set.
> > - EPT violation on that GPA.
> > - VMM figures out the faulted GPA is for MMIO.
> > - VMM clears the suppress #VE bit.
> > - Guest TD gets #VE, and converts MMIO access into MMIO hypercall.
> > - If the GPA maps guest memory, VMM resolves it with guest pages.
> >
> > For both cases, SPTE needs suppress #VE" bit set initially when it
> > is allocated or zapped, therefore non-zero non-present value for SPTE
> > needs to be allowed.
> >
> > This change requires to update FNAME(sync_page) for shadow EPT.
> > "if(!sp->spte[i])" in FNAME(sync_page) means that the spte entry is the
> > initial value. With the introduction of shadow_nonpresent_value which can
> > be non-zero, it doesn't hold any more. Replace zero check with
> > "!is_shadow_present_pte() && !is_mmio_spte()".
>
> I don't think you need to mention above paragraph. It's absolutely unclear how
> is_mmio_spte() will be impacted by this patch by reading above paragraphs.
>
> From the "execution flow" you mentioned above, you will change MMIO fault from
> EPT misconfiguration to EPT violation (in order to get #VE), so theoretically
> you may effectively disable MMIO caching, in which case, if I understand
> correctly, is_mmio_spte() always returns false.
>
> I guess you can just change to check:
>
> if (sp->spte[i] != shadow_nonpresent_value)
>
> Anyway, IMO you can just comment in the code.
>
> After all, what is shadow_nonpresent_value, given you haven't explained what it
> is?
I'll drop the paragraph and add a comment on the code.
> > TDP MMU uses REMOVED_SPTE = 0x5a0ULL as special constant to indicate the
> > intermediate value to indicate one thread is operating on it and the value
> > should be semi-arbitrary value. For TDX (more correctly to use #VE), the
> > value should include suppress #VE value which is SHADOW_NONPRESENT_VALUE.
>
> What is SHADOW_NONPRESENT_VALUE?
>
> > Rename REMOVED_SPTE to __REMOVED_SPTE and define REMOVED_SPTE as
> > SHADOW_NONPRESENT_VALUE | REMOVED_SPTE to set suppress #VE bit.
>
> Ditto. IMHO you don't even need to mention REMOVED_SPTE in changelog.
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 55 ++++++++++++++++++++++++++++++----
> > arch/x86/kvm/mmu/paging_tmpl.h | 3 +-
> > arch/x86/kvm/mmu/spte.c | 5 +++-
> > arch/x86/kvm/mmu/spte.h | 37 ++++++++++++++++++++---
> > arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++++++-----
> > 5 files changed, 105 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 51306b80f47c..f239b6cb5d53 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -668,6 +668,44 @@ static void walk_shadow_page_lockless_end(struct kvm_vcpu *vcpu)
> > }
> > }
> >
> > +static inline void kvm_init_shadow_page(void *page)
> > +{
> > +#ifdef CONFIG_X86_64
> > + int ign;
> > +
> > + WARN_ON_ONCE(shadow_nonpresent_value != SHADOW_NONPRESENT_VALUE);
> > + asm volatile (
> > + "rep stosq\n\t"
> > + : "=c"(ign), "=D"(page)
> > + : "a"(SHADOW_NONPRESENT_VALUE), "c"(4096/8), "D"(page)
> > + : "memory"
> > + );
> > +#else
> > + BUG();
> > +#endif
> > +}
> > +
> > +static int mmu_topup_shadow_page_cache(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm_mmu_memory_cache *mc = &vcpu->arch.mmu_shadow_page_cache;
> > + int start, end, i, r;
> > + bool is_tdp_mmu = is_tdp_mmu_enabled(vcpu->kvm);
> > +
> > + if (is_tdp_mmu && shadow_nonpresent_value)
> > + start = kvm_mmu_memory_cache_nr_free_objects(mc);
> > +
> > + r = kvm_mmu_topup_memory_cache(mc, PT64_ROOT_MAX_LEVEL);
> > + if (r)
> > + return r;
> > +
> > + if (is_tdp_mmu && shadow_nonpresent_value) {
> > + end = kvm_mmu_memory_cache_nr_free_objects(mc);
> > + for (i = start; i < end; i++)
> > + kvm_init_shadow_page(mc->objects[i]);
> > + }
>
> I think you can just extend this to legacy MMU too, but not only TDP MMU.
>
> After all, before this patch, where have you declared that TDX only supports TDP
> MMU? This is only enforced in:
>
> [PATCH v7 043/102] KVM: x86/mmu: Focibly use TDP MMU for TDX
>
> Which is 7 patches later.
>
> Also, shadow_nonpresent_value is only used in couple of places, while
> SHADOW_NONPRESENT_VALUE is used directly in more places. Does it make more
> sense to always use shadow_nonpresent_value, instead of using
> SHADOW_NONPRESENT_VALUE?
>
> Similar to other shadow values, we can provide a function to let caller
> (VMX/SVM) to decide whether it wants to use non-zero for non-present SPTE.
>
> void kvm_mmu_set_non_present_value(u64 value)
> {
> shadow_nonpresent_value = value;
> }
As you pointed out, those logic is independent of TDP MMU or legacy MMU.
So I'll remove is_tdp_mmu.I'll drop shadwo_nonpresent_value and use
SHADWO_NONPRESENT_VALUE.
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
