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

[Bui Quang Minh <minhquangbui99@gmail.com>]

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)