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

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

[Victor Lira]

Hello,

I came across something that is out of spec behavior in the domain APIC.
Sent IPI's with broadcast destination are broadcast for physical mode but not
for logical mode.

Please give it a check because in 7429bfd50dd7 it seems that it was intentional
to crash the domain if the command was lowest priority + logical + broadcast
dest but it got removed.

relevant commits:
  e03ad58f0b8c
  8a02f12b7751
  7429bfd50dd7

Victor Lira (1):
  x86/vlapic: match broadcasts for logical destination mode

 xen/arch/x86/hvm/vlapic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.51.GIT

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

[Victor Lira]

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 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@amd.com>
---
 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));

--
2.51.GIT

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

[Andrew Cooper]

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@amd.com>
> ---
>  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

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

[Roger Pau Monné]

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@amd.com>
> > ---
> >  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.