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? > Do you think the code here is tricky and hard to read? Well, I completely failed to read it... :) I think changing the existing comment something like this might help... - /* Broadcast to xAPIC mode apics */ + /* Any APIC in xAPIC mode will interpret 0xFF as broadcast */ Coupled with a comment on apic_get_delivery_bitmask() clarifying that it depends on the mode of each APIC it considers — which is obvious enough in retrospect now I read the code and you point it out to me, but empirically, we have to concede that it wasn't obvious *enough* :)
smime.p7s
Description: S/MIME cryptographic signature