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 <sullivan.jame...@gmail.com>
>> ---
>> 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()?)
