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

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-04-23 13:08-0600, James Sullivan:
> On 04/23/2015 08:14 AM, Radim Krčmář wrote:
>> 2015-04-06 17:45-0600, James Sullivan:
>>> 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,
>>> +            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).

Good concern!  In this case, I think it's ok to change semantics, as the
function is static.  All callers are going to be in this module and they
call apic_bus_deliver() right after apic_get_delivery_bitmask() now.

(It's not like we could break interrupt delivery with this change, only
 logging or some other meta-operation would be affected.)

>> (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()?)

What we have now is fine.  I presented them to lift constraints that
might have prevented you from making different decisions;
software mustn't configure them, so we're free to do whatever.

(I think it would be better if they didn't work, but haven't read enough
 QEMU code to know what is the approach here.)