Re: [PATCH v2 2/5] apic: add support for x2APIC mode

[Bui Quang Minh]

On 3/28/23 22:58, Bui Quang Minh wrote:
> On 3/27/23 23:49, David Woodhouse wrote:
> > On Mon, 2023-03-27 at 23:35 +0700, Bui Quang Minh wrote:
> > > On 3/27/23 23:22, David Woodhouse wrote:
> > > > On Mon, 2023-03-27 at 22:45 +0700, Bui Quang Minh wrote:
> > > > > > Maybe I'm misreading the patch, but to me it looks that
> > > > > > if (dest == 0xff) apic_get_broadcast_bitmask() bit applies even in
> > > > > > x2apic mode? So delivering to the APIC with physical ID 255 will be
> > > > > > misinterpreted as a broadcast?
> > > > >
> > > > > In case dest == 0xff the second argument to apic_get_broadcast_bitmask
> > > > > is set to false which means this is xAPIC broadcast
> > > >
> > > > Yeah, but it *isn't* xAPIC broadcast. It's X2APIC unicast to APIC#255.
> > > >
> > > > I think you want (although you don't have 'dev') something like this:
> > > >
> > > >
> > > > static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> > > >                                         uint32_t dest, uint8_t 
> > > > dest_mode)
> > > > {
> > > >       APICCommonState *apic_iter;
> > > >       int i;
> > > >
> > > >       memset(deliver_bitmask, 0x00, max_apic_words * sizeof(uint32_t));
> > > >
> > > >       /* x2APIC broadcast id for both physical and logical (cluster) 
> > > > mode */
> > > >       if (dest == 0xffffffff) {
> > > >           apic_get_broadcast_bitmask(deliver_bitmask, true);
> > > >           return;
> > > >       }
> > > >
> > > >       if (dest_mode == 0) {
> > > >           apic_find_dest(deliver_bitmask, dest);
> > > >           /* Broadcast to xAPIC mode apics */
> > > > -        if (dest == 0xff) {
> > > > +        if (dest == 0xff && is_x2apic_mode(dev)) {
> > > >               apic_get_broadcast_bitmask(deliver_bitmask, false);
> > > >           }
> > > >       } else {
> > >
> > > Hmm, the unicast case is handled in apic_find_dest function, the logic
> > > inside the if (dest == 0xff) is for handling the broadcast case only.
> > > This is because when dest == 0xff, it can be both a x2APIC unicast and
> > > xAPIC broadcast in case we have some CPUs that are in xAPIC and others
> > > are in x2APIC.
> >
> > Ah! Yes, I see it now.
> >
> > Shouldn't apic_get_broadcast_bitmask(… true) add *all* APICs to the
> > mask, regardless of their mode? An APIC which is still in xAPIC mode
> > will only look at the low 8 bits and see 0xFF which it also interprets
> > as broadcast? Or is that not how real hardware behaves?
>
> This is interesting. Your point looks reasonable to me but I don't know 
> how to verify it, I'm trying to write kernel module to test it but there 
> are just too many things running on Linux that uses interrupt so the 
> system hangs.
>
> This raises another question: when dest == 0x102 in IPI, does the xAPIC 
> mode CPU with APIC ID 0x2 accept the IPI? I can't see this stated 
> clearly in the Intel SDM.

I do some more testing on my hardware, your point is correct when dest 
== 0xffffffff, the interrupt is delivered to all APICs regardless of 
their mode. And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID 
0x2 also accepts that IPI.