Re: [Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery

[James Sullivan]

On 04/23/2015 08:14 AM, Radim Krčmář wrote:
> 2015-04-06 17:45-0600, James Sullivan:
>> Added argument to apic_get_delivery_bitmask() for msi_redir_hint,
>> and changed calls to the function accordingly (using 0 as a default
>> value for non-MSI interrupts).
>>
>> Modified the implementation of apic_get_delivery_bitmask() to account
>> for the RH bit of an MSI IRQ. The RH bit indicates that the message
>> should target only the lowest-priority processor among those specified
>> by the logical destination of the IRQ.
>>
>> Signed-off-by: James Sullivan <address@hidden>
>> ---
>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
>> @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t 
>> *deliver_bitmask,
>>          }
>>      } else {
>>          /* XXX: cluster mode */
>> +        int l = -1;
>>          memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
>>          for(i = 0; i < MAX_APICS; i++) {
>>              apic_iter = local_apics[i];
>> +            if (!apic_iter) {
>> +                break;
>> +            }
> 
> (I wonder if QEMU would allow
>   'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {')
> 
>> +            if (apic_match_dest(apic_iter, dest_mode, dest)) {
>> +                if (msi_redir_hint) {
> 
> You could check for APIC_DM_LOWPRI here as well and save the
> apic_lowest_prio() loop in patch [1/4].
> LOWPRI would be delivered like FIXED.
> 

I was considering doing that, saving the loop is a big plus. My concern
was that it would change get_delivery_bitmask's semantics in a way that
could be confusing (i.e. it is currently expected that the caller is
responsible for doing arbitration after the fact, this would flip that
responsibility).

>> +                    if (l < 0 ||
>> +                        apic_compare_prio(apic_iter, local_apics[l]) < 0) {
>> +                        l = i;
> 
> (Btw. lowest priority has a lot of cases that are forbidden ...
>  - in combination with physical broadcast
>  - in combination with clustered logical broadcast
>  - to invalid/disabled destinations
> 
>  These most likely won't work correctly in real hardware.
>  Lowest priority is a bad concept for large systems, which is why Intel
>  stopped implementing it.)
> 

Checking for such illegal cases should probably happen in the delivery
routines before we set the delivery bitmask (in apic_bus_deliver()?)