Re: [PATCH] KVM: SVM: make svm_flush_tlb_gva do a full asid flush if NPT enabled

[Paolo Bonzini]

Il sab 25 lug 2026, 00:36 Sean Christopherson <seanjc@xxxxxxxxxx> ha scritto:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index b517257a6315..eca04d4b974e 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1751,7 +1751,7 @@ struct kvm_x86_ops {
> >        * Can potentially get non-canonical addresses through INVLPGs, which
> >        * the implementation may choose to ignore if appropriate.
> >        */
> > -     void (*flush_tlb_gva)(struct kvm_vcpu *vcpu, gva_t addr);
> > +     void (*flush_tlb_gva)(struct kvm_vcpu *vcpu, gva_t addr, bool *full);
>
> LOL, why on earth are you using an out-param?  If we do this at runtime, just
> return a bool, at least that way we don't have to churn every call-site.

To be precise the churn is only one call site out of two, and in fact
it's the one which is mangled even more by your proposal below. :)
More seriously, I used an out parameter because returning 0/1 or
false/true is impenetrable for the Hyper-V call site, while
0/-EOPNOTSUPP (i.e. do not flush at all on NPT) would force changes in
kvm_mmu_invalidate_addr().

So, instead of making things good for one call site at the expense of
the other, the optional out param is more self documenting in svm.c
and is either good or bearable for the callers: Hyper-V doesn't care
either way (it doesn't e.g. need the return value within an "if" or
"while"), and kvm_mmu_invalidate_addr() just gets an extra NULL
argument.

Overall it's a matter of taste, I understand if you're not convinced
but I can say it wasn't out of a whim. In your defense maybe I should
have put this argument in the commit message somewhere? AI also
complained, but I shelved it because "Sean surely has better taste
than the AI"... :)

> But I would much rather handle this by nuking .flush_tlb_gva at setup, e.g.
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index f0144ae8d891..bf16119640dc 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6555,7 +6555,10 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>                 if (is_noncanonical_invlpg_address(addr, vcpu))
>                         return;
>
> -               kvm_x86_call(flush_tlb_gva)(vcpu, addr);
> +               if (kvm_x86_ops.flush_tlb_gva)
> +                       kvm_x86_call(flush_tlb_gva)(vcpu, addr);
> +               else
> +                       kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
>         }
>
>         if (!mmu->sync_spte)

This is a variation on the -EOPNOTSUPP convention, for which I didn't
like having this "else" in the caller that doesn't already have a
full-flush fallback. If you insist, I guess I would be fine with
adding a kvm_flush_tlb_gva() wrapper, use it in mmu.c, and check for
NULL in hyperv.c... but in the end is it really better than the out
param version?

In fact I am not even sure it is better than returning -EOPNOTSUPP.
>From the svm.c point of view it certainly is very clean, but for
everyone else it's super easy to forget about checking the callback;
and you don't even have __must_check to save you.

Paolo