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. Maybe we should set irq.delivery_mode only if RH==1? > > BTW, irq_comm.c is surely the wrong place for all this IA32-specific > interpretation of MSI address and data. And we have yet another > guest-triggerable printk in kvm_irq_delivery_to_apic (messages to > physical ID 0xff). > > Jan > -- Sasha. -- 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
