Re: [PATCH RESEND v4 5/9] KVM: VMX: Add init/set/get functions for SPP

[Yang Weijiang]

On Wed, Aug 14, 2019 at 02:43:39PM +0200, Vitaly Kuznetsov wrote:
> Yang Weijiang <weijiang.yang@xxxxxxxxx> writes:
> 
> > init_spp() must be called before {get, set}_subpage
> > functions, it creates subpage access bitmaps for memory pages
> > and issues a KVM request to setup SPPT root pages.
> >
> > kvm_mmu_set_subpages() is to enable SPP bit in EPT leaf page
> > and setup corresponding SPPT entries. The mmu_lock
> > is held before above operation. If it's called in EPT fault and
> > SPPT mis-config induced handler, mmu_lock is acquired outside
> > the function, otherwise, it's acquired inside it.
> >
> > kvm_mmu_get_subpages() is used to query access bitmap for
> > protected page, it's also used in EPT fault handler to check
> > whether the fault EPT page is SPP protected as well.
> >
> > Co-developed-by: He Chen <he.chen@xxxxxxxxxxxxxxx>
> > Signed-off-by: He Chen <he.chen@xxxxxxxxxxxxxxx>
> > Co-developed-by: Zhang Yi <yi.z.zhang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Zhang Yi <yi.z.zhang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  18 ++++
> >  arch/x86/include/asm/vmx.h      |   2 +
> >  arch/x86/kvm/mmu.c              | 160 ++++++++++++++++++++++++++++++++
> >  arch/x86/kvm/vmx/vmx.c          |  48 ++++++++++
> >  arch/x86/kvm/x86.c              |  40 ++++++++
> >  include/linux/kvm_host.h        |   4 +-
> >  include/uapi/linux/kvm.h        |   9 ++
> >  7 files changed, 280 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 44f6e1757861..5c4882015acc 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -398,8 +398,13 @@ struct kvm_mmu {
> >  	void (*invlpg)(struct kvm_vcpu *vcpu, gva_t gva, hpa_t root_hpa);
> >  	void (*update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> >  			   u64 *spte, const void *pte);
> > +	int (*get_subpages)(struct kvm *kvm, struct kvm_subpage *spp_info);
> > +	int (*set_subpages)(struct kvm *kvm, struct kvm_subpage *spp_info);
> > +	int (*init_spp)(struct kvm *kvm);
> > +
> >  	hpa_t root_hpa;
> >  	gpa_t root_cr3;
> > +	hpa_t sppt_root;
> 
> (I'm sorry if this was previously discussed, I didn't look into previous
> submissions).
> 
> What happens when we launch a nested guest and switch vcpu->arch.mmu to
> point at arch.guest_mmu? sppt_root will point to INVALID_PAGE and SPP
> won't be enabled in VMCS?
> 
> (I'm sorry again, I'm likely missing something obvious)
> 
> -- 
> Vitaly
Hi, Vitaly,
After looked into the issue and others, I feel to make SPP co-existing
with nested VM is not good, the major reason is, L1 pages protected by
SPP are transparent to L1 VM, if it launches L2 VM, probably the
pages would be allocated to L2 VM, and that will bother to L1 and L2.
Given the feature is new and I don't see nested VM can benefit
from it right now, I would like to make SPP and nested feature mutually
exclusive, i.e., detecting if the other part is active before activate one
feature,what do you think of it? 
thanks!