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.
