Re: [XEN PATCH v1 1/1] x86/vlapic: match broadcasts for logical destination mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 15, 2026 at 02:01:12AM +0000, Andrew Cooper wrote:
> On 14/01/2026 11:55 pm, Victor Lira wrote:
> > amd64 vol 3
> > 16.6.1: "In both the flat model and the cluster model, if the
> > destination field = FFh, the interrupt is accepted by all local APICs."
> > 16.14: "A DEST value of FFFF_FFFFh in the ICR is used to broadcast
> > IPIs to all local APICs."
> >
> > intel 64 vol 3
> > 12.6.2.2: "Broadcast to all local APICs is achieved by setting all
> > destination bits to one."
> > 12.12.9: "A destination ID value of FFFF_FFFFH is used
> > for broadcast of interrupts in both logical destination and physical
> > destination modes."
> 
> The formatting here really needs some work.
> 
> >
> > The specs say 0xFFFFFFFF/0xFF should be a broadcast to all APICs in
> > logical destination mode but it is matched only for cluster 0xFFFF/0xFF
> > (or as flat mode in xAPIC).
> >
> > Add a check in vlapic_match_dest similar to what is done for physical
> > destination mode.
> >

This possibly needs a "Fixes:" tag, I think:

Fixes: 7429bfd50dd7 ("This patch provide local APIC support for vmx guest.")

It's been broken since it was introduced.

> > Signed-off-by: Victor Lira <victorm.lira@xxxxxxx>
> > ---
> >  xen/arch/x86/hvm/vlapic.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 79697487ba..1208cd21f0 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -248,7 +248,8 @@ bool vlapic_match_dest(
> >      {
> >      case APIC_DEST_NOSHORT:
> >          if ( dest_mode )
> > -            return vlapic_match_logical_addr(target, dest);
> > +            return (dest == _VLAPIC_ID(target, 0xffffffffU)) ||
> > +                   vlapic_match_logical_addr(target, dest);
> >          return (dest == _VLAPIC_ID(target, 0xffffffffU)) ||
> >                 (dest == VLAPIC_ID(target));
> 
> The SDM/APM quotes are clear, but I think this logic has more bugs than
> just this.
> 
> First, you've got a common expression that the compiler cannot optimise
> because of the function calls hidden in VLAPIC_ID().  The appropriate
> rearrangement would be:
> 
>     case APIC_DEST_NOSHORT:
>         return (dest == _VLAPIC_ID(target, 0xffffffffU) ||
>                 dest_mode ? vlapic_match_logical_addr(target, dest)
>                           : dest == VLAPIC_ID(target));
> 
> 
> However, the first clause looking for the broadcast ID is surely wrong.
> 
> Surely it needs checking against the source LAPIC, not the target. 

That would be:

Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")

> Whether 0xff or 0xffffffff is the broadcast address depends on the xAPIC
> vs x2APIC mode of the sending LAPIC only, and it's surely buggy to fail
> to match targets just because they're in the opposite mode.  So, I think
> the correct code is:
> 
>     case APIC_DEST_NOSHORT:
>         return (dest == _VLAPIC_ID(source, 0xffffffffU) ||
>                 dest_mode ? vlapic_match_logical_addr(target, dest)
>                           : dest == VLAPIC_ID(target));
> 
> 
> Thoughts?

LGTM.

Thanks, Roger.