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,
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?
Best regards,
Maxim Levitsky
