Re: [PATCH v2] x86/vlapic: Don't change ICR on x2APIC SELF_IPI write

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 28/10/2026 à 12:16, Jan Beulich a écrit :
> On 26.10.2026 15:03, Teddy Astie wrote:
> > The Intel SDM (Vol 3A-13.12.11, June 2026) says
> >
> > > The semantics of sending a self-IPI via the SELF IPI register
> > > are identical to sending a self targeted edge triggered fixed
> > > interrupt with the specified vector.
> > However, this doesn't require altering the ICR register.
> > On most Intel processors (at least Core/P-Core), it does alter ICR,
> > but it's not the case on AMD and Atom/E-Core ones, and APICv Self-IPI
> > appears to follow a similar behavior.
> >
> > For consistency, adjust the Xen behavior to send the Self-IPI
> > directly instead of emulating it by modifying ICR register.
> >
> > It also clarify the logic.
> >
> > Assisted-by: Claude Fable 5.1
> > Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> However, ...
>
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -970,11 +970,13 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int 
> > offset)
> >   
> >       if ( vlapic_x2apic_mode(vlapic) )
> >       {
> > -        if ( offset != APIC_SELF_IPI )
> > -            return X86EMUL_UNHANDLEABLE;
> ... why the extra churn here? The overall hunk here ...
>
> > +        if ( offset == APIC_SELF_IPI )
> > +        {
> > +            vlapic_ipi(vlapic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
> > +            return X86EMUL_OKAY;
> > +        }
> >   
> > -        offset = APIC_ICR;
> > -        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
> > +        return X86EMUL_UNHANDLEABLE;
> >       }
> >   
> >       vlapic_reg_write(v, offset, val);
> ... could be
>
> @@ -973,8 +973,8 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
>           if ( offset != APIC_SELF_IPI )
>               return X86EMUL_UNHANDLEABLE;
>   
> -        offset = APIC_ICR;
> -        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
> +        vlapic_ipi(vlapic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
> +        return X86EMUL_OKAY;
>       }
>   
>       vlapic_reg_write(v, offset, val);
> Much closer to ...
>
> > @@ -1050,9 +1052,8 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, 
> > uint64_t val)
> >       case APIC_SELF_IPI:
> >           if ( val & ~APIC_VECTOR_MASK )
> >               return X86EMUL_EXCEPTION;
> > -        offset = APIC_ICR;
> > -        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
> > -        break;
> > +        vlapic_ipi(vlapic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
> > +        return X86EMUL_OKAY;
> >   
> >       case APIC_EOI:
> >       case APIC_ESR:
> ... this hunk then. (I may take the liberty of making that adjustment while
> committing, unless you provide a reason for the bigger change.)
No strong opinion on it nor specific reason.

To me, given that we don't update ICR anymore and no longer need to 
fallthrough the rest of the register write logic, we can invert the 
check to highlight the SELF_IPI case.
> Jan
Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature