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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.
>
> 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. 
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?

Given this bug, I expect the OSes which use fully broadcast interrupts
tend to do using shorthands, not the broadcast destination.

~Andrew