Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[James Sullivan <sullivan.james.f@gmail.com>]

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