On Thu, 2011-09-01 at 20:45 +0300, Gleb Natapov wrote: > On Thu, Sep 01, 2011 at 07:05:35PM +0300, 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> > > --- > > arch/x86/kvm/lapic.c | 17 +++++++++++++++-- > > include/linux/kvm_types.h | 1 + > > virt/kvm/ioapic.h | 2 +- > > virt/kvm/irq_comm.c | 14 +++++++++++--- > > 4 files changed, 28 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 57dcbd4..3347f9f 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -33,6 +33,7 @@ > > #include <asm/page.h> > > #include <asm/current.h> > > #include <asm/apicdef.h> > > +#include <asm/msidef.h> > > #include <linux/atomic.h> > > #include "kvm_cache_regs.h" > > #include "irq.h" > > @@ -325,7 +326,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda) > > } > > > > int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source, > > - int short_hand, int dest, int dest_mode) > > + int short_hand, int dest, int dest_mode, int rh) > > { > > int result = 0; > > struct kvm_lapic *target = vcpu->arch.apic; > > @@ -335,9 +336,21 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source, > > target, source, dest, dest_mode, short_hand); > > > > ASSERT(target); > > + > > + /* If RH bit is not set, use addressing mode from ICR */ > > + if (rh == 0) { > > + u32 icr_low = apic_get_reg(target, APIC_ICR); > > + > > + dest_mode = icr_low & APIC_DEST_MASK; > > Why is that? This function is called for messages generated by > ioapic/ipi/msi. In all those cases destination mode does not depend on > the content of icr_low register in the destination processor. This > function gets correct dest_mode as a parameter. > According to the spec: "If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used" As far as I understand it, dest_mode parameter is invalid if the RH bit is set. > > + > > + /* dest apic can't be 0xff if targeting specific processor */ > > + BUG_ON(dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL && > > + (u8)dest == 0xff); > Can't this be triggered by a guest? I wrote this because of this line: "When RH is 1 and the physical destination mode is used, the Destination ID field must not be set to 0xFF" I did get this check in the wrong location, it was supposed to be in the 'else' case. > > > + } > > + > > switch (short_hand) { > > case APIC_DEST_NOSHORT: > > - if (dest_mode == 0) > > + if (dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL) > This is apic code, why MSI defines? > You're right - I'll fix that. > > /* Physical mode. */ > > result = kvm_apic_match_physical_addr(target, dest); > > else > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h > > index fa7cc72..55d4d9a 100644 > > --- a/include/linux/kvm_types.h > > +++ b/include/linux/kvm_types.h > > @@ -65,6 +65,7 @@ struct kvm_lapic_irq { > > u32 trig_mode; > > u32 shorthand; > > u32 dest_id; > > + u32 redir_hint; > > }; > > > > struct gfn_to_hva_cache { > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h > > index 0b190c3..fd3bb64 100644 > > --- a/virt/kvm/ioapic.h > > +++ b/virt/kvm/ioapic.h > > @@ -68,7 +68,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm) > > } > > > > int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source, > > - int short_hand, int dest, int dest_mode); > > + int short_hand, int dest, int dest_mode, int rh); > > int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2); > > void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode); > > int kvm_ioapic_init(struct kvm *kvm); > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c > > index 9f614b4..e53fc83 100644 > > --- a/virt/kvm/irq_comm.c > > +++ b/virt/kvm/irq_comm.c > > @@ -68,6 +68,11 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e, > > return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level); > > } > > > > +static inline bool kvm_is_rh_lowest_prio(struct kvm_lapic_irq *irq) > > +{ > > + return irq->redir_hint == MSI_ADDR_REDIRECTION_LOWPRI; > > +} > > + > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) > > { > > #ifdef CONFIG_IA64 > > @@ -93,10 +98,12 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, > > continue; > > > > if (!kvm_apic_match_dest(vcpu, src, irq->shorthand, > > - irq->dest_id, irq->dest_mode)) > > + irq->dest_id, irq->dest_mode, > > + irq->redir_hint)) > > continue; > > > > - if (!kvm_is_dm_lowest_prio(irq)) { > > + if (!kvm_is_dm_lowest_prio(irq) && > > + !kvm_is_rh_lowest_prio(irq)) { > > if (r < 0) > > r = 0; > > r += kvm_apic_set_irq(vcpu, irq); > > @@ -128,13 +135,14 @@ 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.redir_hint = (1 << MSI_ADDR_REDIRECTION_SHIFT) & e->msi.address_lo; > > irq.dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo; > > irq.trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data; > > irq.delivery_mode = e->msi.data & 0x700; > > irq.level = 1; > > irq.shorthand = 0; > > > > - /* TODO Deal with RH bit of MSI message address */ > > return kvm_irq_delivery_to_apic(kvm, NULL, &irq); > > } > > > Reading the spec it looks like it will be enough to set irq.delivery_mode > to lowest priority if RH bit is set and destination mode is logical. The > change will be contained in kvm_set_msi() function. Do I miss something? That was my plan initially, the problem I stumbled on was that quote that I've pasted before: "If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used" Which means that I can't use the DM bit from the address register, and have to find the destination processor based on actual addressing model. -- Sasha. -- 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