On 03/10/2015 08:47 AM, Radim Krčmář wrote:
...
>> +    /*
>> +     * Set dest_mode to logical just in case both the RH and DM
>> +     * bits are set, otherwise default to physical.
>> +     */
>> +    phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
>> +                    MSI_ADDR_DEST_MODE_LOGICAL)) !=
>> +            (MSI_ADDR_REDIRECTION_LOWPRI |
>> +                    MSI_ADDR_DEST_MODE_LOGICAL));
>> +    irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
> 
> (Should be APIC_DEST_LOGICAL.  All works because it is a boolean and we
>  only checked for APIC_DEST_PHYSICAL, which is 0.)
> 

Thank you, that should be as follows:

    irq->dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);

?

>>      irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
>>      irq->delivery_mode = e->msi.data & 0x700;
>>      irq->level = 1;
>>      irq->shorthand = 0;
>> -    /* TODO Deal with RH bit of MSI message address */
> 
> RH bit still isn't deal with -- we do not use lowest-priority-like
> delivery in logical destination mode ...

Just want to make sure I understand this comment-
Isn't low-pri delivery mode used in kvm_irq_delivery_to_apic_fast
when irq->dest_mode > APIC_DEST_PHYSICAL (ie, logical)? (See below.)

Do you mean that this patch will interfere with this? As long as
irq->dest_mode is still APIC_DEST_LOGICAL this shouldn't change.


bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
                struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
{
.....
        if (irq->dest_mode == APIC_DEST_PHYSICAL) {
                if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
                        goto out;

                dst = &map->phys_map[irq->dest_id];
        } else {
                u32 mda = irq->dest_id << (32 - map->ldr_bits);
                u16 cid = apic_cluster_id(map, mda);

                if (cid >= ARRAY_SIZE(map->logical_map))
                        goto out;

                dst = map->logical_map[cid];

                bitmap = apic_logical_id(map, mda);

                if (irq->delivery_mode == APIC_DM_LOWEST) {
                        int l = -1;
                        for_each_set_bit(i, &bitmap, 16) {
                                if (!dst[i])
                                        continue;
                                if (l < 0)
                                        l = i;
                                else if (kvm_apic_compare_prio(dst[i]->vcpu, 
dst[l]->vcpu) < 0)
                                        l = i;
                        }

                        bitmap = (l >= 0) ? 1 << l : 0;
                }
        }
.....
}

> How does DM=1/RH=1 work on real hardware?
> (There seem to be interesting corner cases with irq->delivery_mode like
>  APIC_DM_NMI.)
> 

The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode
similarly to other IPIs. I don't believe this patch introduces any invalid
settings listed in section 10-21, Vol. 3, so this shouldn't create any 
weirdness.

Thanks
-James
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to