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 <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"
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 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.
--
Gleb.
--
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
