On Wed, 29 Mar 2023 22:30:44 +0700 Bui Quang Minh <minhquangbu...@gmail.com> wrote:
> On 3/29/23 21:53, Bui Quang Minh wrote: > > 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. > > To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI > destination mode is physical. In case the destination mode is logical, > flat model/cluster model rule applies to determine if the xAPIC CPUs > accept the IPI. Wow, this is so complicated :) It would be nice if you could update apic kvm unit test with your findings if it doesn't test those variants yet. > > > > And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID > > 0x2 also accepts that IPI. >