On 2011-09-02 16:44, Gleb Natapov wrote: > On Fri, Sep 02, 2011 at 04:36:46PM +0200, Jan Kiszka wrote: >> 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 <[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. >>>> >>> >>> 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. >> > Patching them how? To behave exactly like delivery_mode == APIC_DM_LOWEST in > case RH bit is set? Then setting delivery_mode to APIC_DM_LOWEST will > achieve the same goal.
/Wrt selecting the target CPU, but not regarding the vector type delivered to that CPU (think of obscure things like RH=1, delivery_mode=APIC_DM_NMI). If RH=1 only meant hard-wiring delivery_mode to single value, then this would be redundant encoding. And that's always suspicious (unless there is legacy involved). 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
