Re: [RFC PATCH 01/13] KVM: nSVM: Track the ASID per-VMCB

[Yosry Ahmed]

On Fri, Feb 28, 2025 at 08:23:48PM -0500, Maxim Levitsky wrote:
> On Wed, 2025-02-05 at 18:23 +0000, Yosry Ahmed wrote:
> > The ASID is currently tracked per-vCPU, because the same ASID is used by
> > L1 and L2. That ASID is flushed on every transition between L1 and L2.
> > 
> > Track the ASID separately for each VMCB (similar to the
> > asid_generation), giving L2 a separate ASID. This is in preparation for
> > doing fine-grained TLB flushes on nested transitions instead of
> > unconditional full flushes.
> > 
> > The ASIDs are still not fully maintained (e.g. a remote flush will only
> > flush the current ASID), so keep the TLB flush on every transition until
> > this is sorted out.
> > 
> > L1's ASID will be flushed on KVM_REQ_TLB_FLUSH_GUEST if it is the
> > active context, so remove the TODO in nested_svm_transition_tlb_flush()
> > about it.
> > 
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > ---
> >  arch/x86/kvm/svm/nested.c |  1 -
> >  arch/x86/kvm/svm/sev.c    |  2 +-
> >  arch/x86/kvm/svm/svm.c    | 12 +++++++-----
> >  arch/x86/kvm/svm/svm.h    |  2 +-
> >  4 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 04c375bf1ac2a..bbe4f3ac9f250 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -495,7 +495,6 @@ static void nested_svm_transition_tlb_flush(struct kvm_vcpu *vcpu)
> >  	 *  - Honor L1's request to flush an ASID on nested VMRUN
> >  	 *  - Sync nested NPT MMU on VMRUN that flushes L2's ASID[*]
> >  	 *  - Don't crush a pending TLB flush in vmcb02 on nested VMRUN
> > -	 *  - Flush L1's ASID on KVM_REQ_TLB_FLUSH_GUEST
> >  	 *
> >  	 * [*] Unlike nested EPT, SVM's ASID management can invalidate nested
> >  	 *     NPT guest-physical mappings on VMRUN.
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 799f8494b599c..b0adfd0537d00 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -3468,7 +3468,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
> >  	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> >  
> >  	/* Assign the asid allocated with this SEV guest */
> > -	svm->asid = asid;
> > +	svm->current_vmcb->asid = asid;
> >  
> >  	/*
> >  	 * Flush guest TLB:
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 7640a84e554a6..08340ae57777b 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1335,8 +1335,10 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
> >  		save->g_pat = vcpu->arch.pat;
> >  		save->cr3 = 0;
> >  	}
> > -	svm->current_vmcb->asid_generation = 0;
> > -	svm->asid = 0;
> > +	svm->vmcb01.asid_generation = 0;
> > +	svm->vmcb01.asid = 0;
> > +	svm->nested.vmcb02.asid_generation = 0;
> > +	svm->nested.vmcb02.asid = 0;
> >  
> >  	svm->nested.vmcb12_gpa = INVALID_GPA;
> >  	svm->nested.last_vmcb12_gpa = INVALID_GPA;
> > @@ -1988,7 +1990,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
> >  	}
> >  
> >  	svm->current_vmcb->asid_generation = sd->asid_generation;
> > -	svm->asid = sd->next_asid++;
> > +	svm->current_vmcb->asid = sd->next_asid++;
> >  }
> >  
> >  static void svm_set_dr6(struct vcpu_svm *svm, unsigned long value)
> > @@ -4235,8 +4237,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> >  
> >  	sync_lapic_to_cr8(vcpu);
> >  
> > -	if (unlikely(svm->asid != svm->vmcb->control.asid)) {
> > -		svm->vmcb->control.asid = svm->asid;
> > +	if (unlikely(svm->current_vmcb->asid != svm->vmcb->control.asid)) {
> > +		svm->vmcb->control.asid = svm->current_vmcb->asid;
> >  		vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> >  	}
> >  	svm->vmcb->save.cr2 = vcpu->arch.cr2;
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index 9d7cdb8fbf872..ebbb0b1a64676 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -133,6 +133,7 @@ struct kvm_vmcb_info {
> >  	unsigned long pa;
> >  	int cpu;
> >  	uint64_t asid_generation;
> > +	u32 asid;
> >  };
> >  
> >  struct vmcb_save_area_cached {
> > @@ -247,7 +248,6 @@ struct vcpu_svm {
> >  	struct vmcb *vmcb;
> >  	struct kvm_vmcb_info vmcb01;
> >  	struct kvm_vmcb_info *current_vmcb;
> > -	u32 asid;
> >  	u32 sysenter_esp_hi;
> >  	u32 sysenter_eip_hi;
> >  	uint64_t tsc_aux;
> 
> Hi,
> 

Hi,

Thanks for taking a look! 

> 
> I think it should be possible to eliminate separate ASID field (current_vmcb->asid/svm->asid)
> completely and instead just use the value stored in the vmcb.
> 
> When there is a need to update it, KVM can also set the corresponding dirty bit
> as done in svm_vcpu_run (new_asid also already does this when the asid generation increases)
> 
> Also KVM already sets the tlb_ctl directly in the vmcb.
> 
> What do you think?

Yeah I think we can do that, although if we go with Sean's suggestion of
a per VM or a per vCPU ASID, this will change anyway. If we use a per
vCPU ASID, I think it would be nice to have it directly in svm->asid and
svm->nested.asid02 to be consistent with VMX.

I will see how the code turns out to be after taking Sean's suggestion
and go from there.