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

[Bui Quang Minh]

On 3/27/23 22:37, David Woodhouse wrote:
> 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?

In case dest == 0xff the second argument to apic_get_broadcast_bitmask 
is set to false which means this is xAPIC broadcast

static void apic_get_broadcast_bitmask(uint32_t *deliver_bitmask,
                                       bool is_x2apic_broadcast)
{
    int i;
    APICCommonState *apic_iter;

    for (i = 0; i < max_apics; i++) {
        apic_iter = local_apics[i];
        if (apic_iter) {
            bool apic_in_x2apic = is_x2apic_mode(&apic_iter->parent_obj);

            if (is_x2apic_broadcast && apic_in_x2apic) {
                apic_set_bit(deliver_bitmask, i);
            } else if (!is_x2apic_broadcast && !apic_in_x2apic) {
                apic_set_bit(deliver_bitmask, i);
            }
        }
    }
}

In apic_get_broadcast_bitmask, because is_x2apic_broadcast == false, the 
delivery bit set only if that apic_in_x2apic == false (that CPU is in 
xAPIC mode)