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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.


>From the Intel side, original VT-x had no acceleration here, but one of
the very early features they added was virtualisation of TPR accesses. 
This covers both CR8 and APIC_TPR, and they both map onto the APIC
backing page without intermediate storage either.


So, the next steps (probably as a prereq to this series) are:

1) Determine the actual behaviour of CR8 vs APIC_TPR on real hardware,
including with the APIC being disabled.  It may well be different
between Intel and AMD
2) Use 1 to fix the synchronisation of V_TPR and APIC_TPR so we're not
writing V_TPR on every VMExit
3) Testing in place to confirm we don't regress.  I should dust off my
ESR test as a starting point.

In particular I note that disabling the APIC needs to disable all parts
of APIC acceleration, and that the Intel vmx_vlapic_msr_changed() hook
doesn't do this correctly for TPR.  I also observe there's a major
difference between the Intel hook and what you add in this series,
insofar as the x2APIC MSR intercepts don't get altered, which is suspicious.

Thanks,

~Andrew