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

[David Woodhouse]

On Mon, 2023-03-27 at 22:33 +0700, Bui Quang Minh wrote:
> 
> > > +    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) {
> > 
> > Might be nice to have a constant for DEST_MODE_PHYS vs.
> > DEST_MODE_LOGICAL to make this clearer?
> 
> I'll fix it in the next version.
> 
> > > +        apic_find_dest(deliver_bitmask, dest);
> > > +        /* Broadcast to xAPIC mode apics */
> > >            if (dest == 0xff) {
> > > -            memset(deliver_bitmask, 0xff, MAX_APIC_WORDS * 
> > > sizeof(uint32_t));
> > > -        } else {
> > > -            int idx = apic_find_dest(dest);
> > > -            memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * 
> > > sizeof(uint32_t));
> > > -            if (idx >= 0)
> > > -                apic_set_bit(deliver_bitmask, idx);
> > > +            apic_get_broadcast_bitmask(deliver_bitmask, false);
> > 
> > 
> > Hrm... aren't you still interpreting destination 0x000000FF as
> > broadcast even for X2APIC mode? Or am I misreading this?
> 
> In case the destination is 0xFF, the IPI will be delivered to CPU has
> APIC ID 0xFF if it is in x2APIC mode, and it will be delivered to all
> CPUs that are in xAPIC mode. In case the destination is 0xFFFFFFFF, the 
> IPI is delivered to all CPUs that are in x2APIC mode. I've created 
> apic_get_broadcast_bitmask function and changed the apic_find_dest to
> implement that logic.

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?

smime.p7s
Description: S/MIME cryptographic signature