Re: [PATCH v1 11/28] x86/hvm: introduce vcpu_is_vlapic_tpr_emulated()

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 11/6/26 1:36 PM, Andrew Cooper wrote:
> On 05/11/2026 1:33 am, dmukhin@xxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Introduce vcpu_is_vlapic_tpr_emulated() in genetic SVM code to account
> > for the case when platform provides accelerated access to vLAPIC's TPR.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > ---
> >   xen/arch/x86/hvm/svm/svm.c | 38 +++++++++++++++++++++++++++-----------
> >   1 file changed, 27 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 673c818a0039..14a86ca4ff8f 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -925,6 +925,19 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> >       }
> >   }
> >   
> > +static bool vcpu_is_vlapic_tpr_emulated(const struct vcpu *v, bool force_noaccel)
> > +{
> > +    const struct vlapic *vlapic = vcpu_vlapic(v);
> > +
> > +    if ( force_noaccel )
> > +        return false;
> > +
> > +    if ( vlapic_hw_disabled(vlapic) )
> > +        return false;
> > +
> > +    return true;
> > +}
> > +
> >   static void noreturn cf_check svm_do_resume(void)
> >   {
> >       struct vcpu *v = current;
> > @@ -933,7 +946,6 @@ static void noreturn cf_check svm_do_resume(void)
> >                           v->domain->arch.monitor.software_breakpoint_enabled ||
> >                           v->domain->arch.monitor.debug_exception_enabled);
> >       bool vcpu_guestmode = false;
> > -    struct vlapic *vlapic = vcpu_vlapic(v);
> >   
> >       if ( nestedhvm_enabled(v->domain) && nestedhvm_vcpu_in_guestmode(v) )
> >           vcpu_guestmode = 1;
> > @@ -959,8 +971,9 @@ static void noreturn cf_check svm_do_resume(void)
> >           vmcb->cleanbits.raw = 0;
> >       }
> >   
> > -    if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) )
> > +    if ( vcpu_is_vlapic_tpr_emulated(v, vcpu_guestmode) )
> >       {
> > +        struct vlapic *vlapic = vcpu_vlapic(v);
> >           vintr_t intr;
> >   
> >           /* Reflect the vlapic's TPR in the hardware vtpr */
> > @@ -2538,7 +2551,6 @@ void asmlinkage svm_vmexit_handler(void)
> >       int insn_len;
> >       vintr_t intr;
> >       bool vcpu_guestmode = false;
> > -    struct vlapic *vlapic = vcpu_vlapic(v);
> >   
> >       regs->rax = vmcb->rax;
> >       regs->rip = vmcb->rip;
> > @@ -2565,8 +2577,10 @@ void asmlinkage svm_vmexit_handler(void)
> >        * NB. We need to preserve the low bits of the TPR to make checked builds
> >        * of Windows work, even though they don't actually do anything.
> >        */
> > -    if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) )
> > +    if ( vcpu_is_vlapic_tpr_emulated(v, vcpu_guestmode) )
> >       {
> > +        struct vlapic *vlapic = vcpu_vlapic(v);
> > +
> >           intr = vmcb_get_vintr(vmcb);
> >           vlapic_set_reg(vlapic, APIC_TASKPRI,
> >                      ((intr.fields.tpr & 0x0F) << 4) |
> > @@ -3097,14 +3111,16 @@ void asmlinkage svm_vmexit_handler(void)
> >       }
> >   
> >     out:
> > -    if ( vcpu_guestmode || vlapic_hw_disabled(vlapic) )
> > -        return;
> > +    if ( vcpu_is_vlapic_tpr_emulated(v, vcpu_guestmode ) )
> > +    {
> > +        struct vlapic *vlapic = vcpu_vlapic(v);
> >   
> > -    /* The exit may have updated the TPR: reflect this in the hardware vtpr */
> > -    intr = vmcb_get_vintr(vmcb);
> > -    intr.fields.tpr =
> > -        (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
> > -    vmcb_set_vintr(vmcb, intr);
> > +        /* The exit may have updated the TPR: reflect this in the hardware 
> > vtpr */
> > +        intr = vmcb_get_vintr(vmcb);
> > +        intr.fields.tpr =
> > +            (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
> > +        vmcb_set_vintr(vmcb, intr);
> > +    }
> There are a large number of bugs here; they're prexisting, but this
> isn't helping.
>
> First, these final two hunks are wonky.  They're necessary, but only
> because of bugs elsewhere in Xen.
>
> In order to explain, I need to cover one of the bigger sins of the AMD64
> architecture, CR8.  See APM Vol2 8.5.2 External Interrupt Priorities.
>
> Windows (at least used to) write APIC_TPR very frequently.  Microsoft
> made demands of the hardware vendors, and AMD made CR8.  AMD invented a
> register called TPR which is an architectural abstraction of APIC_TPR,
> and defined it's access to be via CR8.  They even made LOCK MOV CR8 (an
> otherwise illegal encoding) be an alias of MOV CR8, for 32bit software
> to use.
>
> AMD state that it is undefined how TPR and APIC_TPR interact.  Intel
> stated that CR8 maps directly on to APIC_TPR, and that the APIC must be
> enabled for this mechanism to work.  AMD do have storage for TPR which
> is separate to APIC_TPR (this has been confirmed), and as a result it
> must be possible to desynchronise TPR and APIC_TPR when the APIC is
> disabled.  Intel's wording suggests that they do not have storage, and
> "SDM Vol2 13.8.6.1 Interaction of Task Priorities between CR8 and APIC"
> hints specifically that only writes are reflected meaning that reads may
> not be.
>
>
> Since the outset, SVM had accelerated CR8.  Furthermore, because the
> V_INTR_MASKING controls both the host/guest EFLAGS.IF split *and* CR8
> virtualisation, we can't run VMs without accelerated CR8.  V_TPR is the
> storage.
>
> This final hunk exists because we have no path from the vLAPIC code
> wiring writes of APIC_TPR back into the V_TPR field.  But, it has the
> side effect of always clearing the TPR cleanbit, and exists only to
> cover an operation where both manuals say "don't mix and match using CR8
> or APIC_TPR;  use one or the other".
>
> When AVIC is active, the V_TPR field ceases to exist and is now mapped
> directly on to the APIC backing page, without intermediate storage.
Not according to the manual... It makes it clear that when AVIC is
enabled V_TPR is still used with bi-directional synchronization
between the CR8/V_TPR and the AVIC backing page:

"When the guest operating system writes to the TPR, the value is updated
in the backing page and the upper 4 bits of the value are automatically
copied by the hardware to the V_TPR value in the VMCB. All reads from
the TPR location return the value from the vAPIC backing page. Also, any
TPR accesses using the MOV CR8 semantics update the backing page and
V_TPR values.
...
The synchronization between the Task Priority field of the TPR and the
Task Priority field of CR8 is normal local APIC behavior which is
emulated by AVIC.
...
Enabling AVIC also affects CR8 behavior independent of V_INTR_MASKING
enable (bit 24): writes to CR8 affect the V_TPR and update the backing
page and reads from CR8 return V_TPR."

I can't find anything about what happens if the value in V_TPR and the
backing page differ during a VMRUN. Are they synchronized again or not?

Ross