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.
> >
> >> 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.
>
Yes, if we are going to pursue that we need Intel to clarify what SDM means.
--
Gleb.
--
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
