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

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

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 :)


> And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID 
> 0x2 also accepts that IPI.