On 2011-09-02 15:13, Sasha Levin wrote:
> On Fri, 2011-09-02 at 14:11 +0200, Jan Kiszka wrote:
>> On 2011-09-02 13:36, Jan Kiszka wrote:
>>> On 2011-09-02 13:27, Jan Kiszka wrote:
>>>> On 2011-09-02 09:48, Sasha Levin wrote:
>>>>> The RH bit exists in the message address register (lower 32 bits of
>>>>> the address).
>>>>>
>>>>> The bit indicates whether the message should go to the processor which was
>>>>> indicated in the destination ID bits, or whether it should go to the
>>>>> processor running at the lowest priority.
>>>>>
>>>>> Cc: Avi Kivity <avi@xxxxxxxxxx>
>>>>> Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>>>>> Signed-off-by: Sasha Levin <levinsasha928@xxxxxxxxx>
>>>>> ---
>>>>> virt/kvm/irq_comm.c | 17 ++++++++++++++++-
>>>>> 1 files changed, 16 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>>>>> index 9f614b4..0ba3a3d 100644
>>>>> --- a/virt/kvm/irq_comm.c
>>>>> +++ b/virt/kvm/irq_comm.c
>>>>> @@ -134,7 +134,22 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>>>>> irq.level = 1;
>>>>> irq.shorthand = 0;
>>>>>
>>>>> - /* TODO Deal with RH bit of MSI message address */
>>>>> + /*
>>>>> + * If the RH bit is set, we'll deliver to the processor running
>>>>> + * at the lowest priority.
>>>>> + */
>>>>> + if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) {
>>>>> + irq.delivery_mode = MSI_DATA_DELIVERY_LOWPRI;
>>>>> + } else {
>>>>> + /*
>>>>> + * If the RH bit is not set, we'll deliver to the specific
>>>>> + * processor mentioned in destination ID, and ignore the DM
>>>>> + * bit.
>>>>> + */
>>>>> + irq.dest_mode = MSI_ADDR_DEST_MODE_PHYSICAL;
>>>>> + irq.delivery_mode = MSI_DATA_DELIVERY_FIXED;
>>>>> + }
>>>>> +
>>>>> return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
>>>>> }
>>>>>
>>>>
>>>> Do you happen have a kvm unit test for this? Or how did you validate the
>>>> change? It doesn't look incorrect to me, I'd just like to check it QEMU
>>>> as well which apparently already has the logic above but also some
>>>> contradictory comment.
>>>
>>> Err, no, QEMU does not have this logic, it also ignores RH.
>>>
>>> But the above bits make "irq.delivery_mode = e->msi.data & 0x700"
>>> pointless. And that strongly suggests something is still wrong.
>>
>> I tend to believe that this is what the spec tries to tell us:
>>
>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>> index 9f614b4..b72f77a 100644
>> --- a/virt/kvm/irq_comm.c
>> +++ b/virt/kvm/irq_comm.c
>> @@ -128,7 +128,8 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>> MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
>> irq.vector = (e->msi.data &
>> MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
>> - irq.dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
>> + irq.dest_mode = ((e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL) &&
>> + (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI));
>> irq.trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
>> irq.delivery_mode = e->msi.data & 0x700;
>> irq.level = 1;
>>
>> ie. the DM flag is only relevant if RH is set, and RH==0 is equivalent
>> to RH==1 && DH==0.
>
> Thing is, the spec specifically states that RH==1 should deliver to
> lowest priority - even though it doesn't state whats the relationship
> between delivery mode and RH bit.
The spec says "When RH is 1 and the physical destination mode is used
[DM=0], the Destination ID field must not be set to 0xFF; it must point
to a processor that is present and enabled to receive the interrupt."
As far as I understand, there is no "lowest prio" in the setup RH=1/DM=0.
I've cc'ed Kevin who just worked on the APIC model. Maybe he can provide
some authoritative answer or dig it up @Intel.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
