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

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

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. Do you think the code here is tricky and hard to read?