linux-kernel - Re: [PATCH 1/2] KVM: SVM: Sync TPR from LAPIC into VMCB::V

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aKkAN1-2gQWCpWx4@google.com>
Date: Fri, 22 Aug 2025 16:41:43 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Maxim Levitsky <mlevitsk@...hat.com>, 
	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>, 
	Alejandro Jimenez <alejandro.j.jimenez@...cle.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Naveen N Rao <naveen@...nel.org>, 
	"Radim Krčmář" <rkrcmar@...hat.com>
Subject: Re: [PATCH 1/2] KVM: SVM: Sync TPR from LAPIC into VMCB::V_TPR when
 setting LAPIC regs

On Sat, Aug 23, 2025, Maciej S. Szmigiero wrote:
> On 21.08.2025 22:38, Sean Christopherson wrote:
> > On Tue, Aug 19, 2025, Maciej S. Szmigiero wrote:
> > > +	/*
> > > +	 * Sync TPR from LAPIC TASKPRI into V_TPR field of the VMCB.
> > > +	 *
> > > +	 * When AVIC is enabled the normal pre-VMRUN sync in sync_lapic_to_cr8()
> > > +	 * is inhibited so any set TPR LAPIC state would not get reflected
> > > +	 * in V_TPR.
> > 
> > Hmm, I think that code is straight up wrong.  There's no justification, just a
> > claim:
> > 
> >    commit 3bbf3565f48ce3999b5a12cde946f81bd4475312
> >    Author:     Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> >    AuthorDate: Wed May 4 14:09:51 2016 -0500
> >    Commit:     Paolo Bonzini <pbonzini@...hat.com>
> >    CommitDate: Wed May 18 18:04:31 2016 +0200
> > 
> >      svm: Do not intercept CR8 when enable AVIC
> >      When enable AVIC:
> >          * Do not intercept CR8 since this should be handled by AVIC HW.
> >          * Also, we don't need to sync cr8/V_TPR and APIC backing page.   <======
> >      Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> >      [Rename svm_in_nested_interrupt_shadow to svm_nested_virtualize_tpr. - Paolo]
> >      Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> > 
> > That claim assumes APIC[TPR] will _never_ be modified by anything other than
> > hardware.  That's obviously false for state restore from userspace, and it's also
> > technically false at steady state, e.g. if KVM managed to trigger emulation of a
> > store to the APIC page, then KVM would bypass the automatic harware sync.
> > 
> > There's also the comically ancient KVM_SET_VAPIC_ADDR, which AFAICT appears to
> > be largely dead code with respect to vTPR (nothing sets KVM_APIC_CHECK_VAPIC
> > except for the initial ioctl), but could again set APIC[TPR] without updating
> > V_TPR.
> > 
> > So, rather than manually do the update during state restore, my vote is to restore
> > the sync logic.  And if we want to optimize that code (seems unnecessary), then
> > we should hook all TPR writes.
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index d9931c6c4bc6..1bfebe40854f 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4046,8 +4046,7 @@ static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
> >          struct vcpu_svm *svm = to_svm(vcpu);
> >          u64 cr8;
> > -       if (nested_svm_virtualize_tpr(vcpu) ||
> > -           kvm_vcpu_apicv_active(vcpu))
> > +       if (nested_svm_virtualize_tpr(vcpu))
> >                  return;
> >          cr8 = kvm_get_cr8(vcpu);
> > 
> > 
> 
> So you want to just do an unconditional LAPIC -> V_TPR sync at each VMRUN
> and not try to patch every code flow where these possibly could get de-synced
> to do such sync only on demand, correct?

Yep.  For a fix, I definitely want to go with the bare minimum.  If we want to
optimize the sync, that can be done on top, and it can be done irrespective of
AVIC.  E.g. for guests that don't modify TPR, the sync is almost pure overhead
too.

> By the way, the original Suravee's submission for the aforementioned patch
> did *not* inhibit that sync when AVIC is on [1].
> 
> Something similar to this sync inhibit only showed in v4 [2],
> probably upon Radim's comment on v3 [3] that:
> > I think we can exit early with svm_vcpu_avic_enabled().
> 
> But the initial sync inhibit condition in v4 was essentially
> nested_svm_virtualize_tpr() && svm_vcpu_avic_enabled(),
> which suggests there was some confusion what was exactly meant
> by the reviewer comment.

Hmm, I suspect that was just a goof.  My guess is that Radim and Suravee simply
forgot to consider TPR writes that aren't handled by hardware.

> The final sync inhibit condition only showed in v5 [4].
> No further discussion happened on that point.
> 
> Thanks,
> Maciej
> 
> [1]: https://lore.kernel.org/kvm/1455285574-27892-9-git-send-email-suravee.suthikulpanit@amd.com/
> [2]: https://lore.kernel.org/kvm/1460017232-17429-11-git-send-email-Suravee.Suthikulpanit@amd.com/
> [3]: https://lore.kernel.org/kvm/20160318211048.GB26119@potion.brq.redhat.com/
> [4]: https://lore.kernel.org/kvm/1462388992-25242-13-git-send-email-Suravee.Suthikulpanit@amd.com/
>