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

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

On 03/11/2015 07:43 AM, Radim Krčmář wrote:
> 2015-03-10 16:39-0600, James Sullivan:
>> On 03/10/2015 08:47 AM, Radim Krčmář wrote:
>>>> +	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);
>> ?
>
> Yes, thanks.  (No need for parentheses around macros, we "agreed" to
> cover that in definition and they don't make this more readable.)
>

I'll tidy that up too.

> Your patch improves the situation, but removing the TODO is not
> warranted -- RH still doesn't do what it should:
>
>>> 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.
>
> Quoting SDM Jan 2015, 10.11.1 Message Address Register Format:
>   This bit [RH] indicates whether the message should be directed to the
>   processor with the lowest interrupt priority among processors that can
>   receive the interrupt.
>
> => it should behave like lowest priority delivery.
> Deliver to just one APIC -- we don't do that.
>
> KVM interrupt delivery functions can only specify lowest priority though
> delivery_mode and we would have to rework KVM if MSI can set something
> else in the delivery_mode (like NMI).
>

I see-- That will pose issues when you want some particular semantics of one
delivery mode (such as ignoring vector info in NMI) but want low-pri delivery.
Short of writing an MSI specific delivery function and calling it in kvm_set_msi,
I can't think of a way to get this. Do you think that's warranted?

I'll document this and resubmit this patch with the TODO for now. Thanks for the
insight.