On 2011-09-02 16:30, Sasha Levin wrote:
> On Fri, 2011-09-02 at 16:25 +0200, Jan Kiszka wrote:
>> On 2011-09-02 16:11, Sasha Levin wrote:
>>> On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote:
>>>> 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."
>>>>
>>>
>>> When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1?
>>
>> irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses
>> kvm_apic_match_logical_addr for filtering out possible target CPUs.
>>
>> Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already
>> doing the right thing, even for delivery_mode != APIC_DM_LOWEST.
>>
>
> The missing part is that when RH=1 we must look for the lowest priority:
>
> "Redirection hint indication (RH) - This bit indicates whether the
> message should be directed to the processor with the lowest interrupt
> priority among processors that can receive the interrupt."
>
> So it's not enough to set dest_mode, we must also make sure that
> delivery_mode is set to low prio when RH=1.
That's debatable. delivery_mode == APIC_DM_LOWEST includes this target
selection, but also more. I have a bad feeling when we just overwrite
delivery_mode as defined by the MSI data field instead of only patching
kvm_irq_delivery_to_apic or kvm_is_dm_lowest_prio - if required.
>
>> Again my question to you: Did you observe unexpected behaviour with some
>> real guests, or is this just based on code and spec study so far? If we
>> had a test case, that could also provide valuable hints.
>
> Sorry, no test case.
>
> I've stumbled on the 'TODO' comment when I was digging into the MSI
> implementation in KVM and decided to implement it based on specs.
Then we definitely need some blessing by Intel to avoid subtle regressions.
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
