On Thu, 2011-09-01 at 22:31 +0300, Gleb Natapov wrote:
> On Thu, Sep 01, 2011 at 09:59:52PM +0300, Sasha Levin wrote:
> > 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 <[email protected]>
> > > > Cc: Marcelo Tosatti <[email protected]>
> > > > Signed-off-by: Sasha Levin <[email protected]>
> > > > ---
> > > > 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"
> So how ID field is interpreted?
>
> >
> > As far as I understand it, dest_mode parameter is invalid if the RH bit
> > is set.
> >
> Intel spec says:
> When RH is 0, the interrupt is directed to the processor listed in the
> Destination ID field.
> Looks like it says that Destination ID should be treated as physical.
>
> But you do not just ignore dest_mode, you take it from icr_low of destination
> cpu and this does not make sense to me.
>
dest_mode is picked up from the DM bit in the message address register,
the spec states that it's invalid - so I'm picking it from the ICR
instead.
It wasn't clear to me that it's simply physical, but I'm not too sure it
isn't either.
> > > > +
> > > > + /* 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.
> >
> You can kill a guest if it does something that is prohibited by the spec,
> or you can stop delivering interrupts to the guest or change random
> memory in the guest, but not Oops a host.
>
> > >
> > > > + }
> > > > +
> > > > 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.
> >
> What is "actual addressing model"? Addressing is per message not some
> global state.
>
By "actual addressing model" I meant checking the ICR of of each APIC to
see if we should address it logically or physically.
> --
> Gleb.
--
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
