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 <[email protected]> >>>>>>> Cc: Marcelo Tosatti <[email protected]> >>>>>>> Signed-off-by: Sasha Levin <[email protected]> >>>>>>> --- >>>>>>> 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. 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. 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
