On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote:
> On 2013-06-24 14:19, Gleb Natapov wrote:
> > This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
> > the commit kvm_apic_set_irq() no longer returns accurate information
> > about interrupt injection status if injection is done into disabled
> > APIC. RTC interrupt coalescing tracking relies on the information to be
> > accurate and cannot recover if it is not.
> >
> > Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 9d75193..9f4bea8 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> > return highest_irr;
> > }
> >
> > -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > - int vector, int level, int trig_mode,
> > - unsigned long *dest_map);
> > +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > + int vector, int level, int trig_mode,
> > + unsigned long *dest_map);
>
> I still think __apic_accept_irq should unconditionally inject, and the
> test for acpi_enabled belongs into kvm_apic_set_irq. Why should
> __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
> See below for another reason to refactor this part of the interface.
>
10.4.7.2 Local APIC State After It Has Been Software Disabled
The local APIC will respond normally to INIT, NMI, SMI, and SIPI
messages.
> >
> > -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> > - unsigned long *dest_map)
> > +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> > + unsigned long *dest_map)
> > {
> > struct kvm_lapic *apic = vcpu->arch.apic;
> >
> > - __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
> > - irq->level, irq->trig_mode, dest_map);
> > + return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
> > + irq->level, irq->trig_mode, dest_map);
> > }
> >
> > static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
> > @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> > *r = -1;
> >
> > if (irq->shorthand == APIC_DEST_SELF) {
> > - kvm_apic_set_irq(src->vcpu, irq, dest_map);
> > - *r = 1;
> > + *r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
> > return true;
> > }
> >
> > @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> > continue;
> > if (*r < 0)
> > *r = 0;
> > - kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
> > - *r += 1;
> > + *r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
> > }
> >
> > ret = true;
> > @@ -664,11 +662,15 @@ out:
> > return ret;
> > }
> >
> > -/* Set an IRQ pending in the lapic. */
> > -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > - int vector, int level, int trig_mode,
> > - unsigned long *dest_map)
> > +/*
> > + * Add a pending IRQ into lapic.
> > + * Return 1 if successfully added and 0 if discarded.
> > + */
> > +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > + int vector, int level, int trig_mode,
> > + unsigned long *dest_map)
> > {
> > + int result = 0;
> > struct kvm_vcpu *vcpu = apic->vcpu;
> >
> > switch (delivery_mode) {
> > @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > if (dest_map)
> > __set_bit(vcpu->vcpu_id, dest_map);
> >
> > - if (kvm_x86_ops->deliver_posted_interrupt)
> > + if (kvm_x86_ops->deliver_posted_interrupt) {
> > + result = 1;
> > kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> > - else {
> > - if (apic_test_and_set_irr(vector, apic)) {
> > + } else {
> > + result = !apic_test_and_set_irr(vector, apic);
>
> This part of the revert makes no sense. If deliver_posted_interrupt is
> on, we don't have this feedback anymore, thus we decided to remove it, no?
>
Agree, but I wanted to do clear revert and fix on top.
> Jan
>
> > +
> > + if (!result) {
> > if (trig_mode)
> > apic_debug("level trig mode repeatedly "
> > "for vector %d", vector);
> > @@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> > }
> > out:
> > trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
> > - trig_mode, vector, false);
> > + trig_mode, vector, !result);
> > break;
> >
> > case APIC_DM_REMRD:
> > @@ -709,12 +714,14 @@ out:
> > break;
> >
> > case APIC_DM_NMI:
> > + result = 1;
> > kvm_inject_nmi(vcpu);
> > kvm_vcpu_kick(vcpu);
> > break;
> >
> > case APIC_DM_INIT:
> > if (!trig_mode || level) {
> > + result = 1;
> > /* assumes that there are only KVM_APIC_INIT/SIPI */
> > apic->pending_events = (1UL << KVM_APIC_INIT);
> > /* make sure pending_events is visible before sending
> > @@ -731,6 +738,7 @@ out:
> > case APIC_DM_STARTUP:
> > apic_debug("SIPI to vcpu %d vector 0x%02x\n",
> > vcpu->vcpu_id, vector);
> > + result = 1;
> > apic->sipi_vector = vector;
> > /* make sure sipi_vector is visible for the receiver */
> > smp_wmb();
> > @@ -752,6 +760,7 @@ out:
> > delivery_mode);
> > break;
> > }
> > + return result;
> > }
> >
> > int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
> > @@ -1461,7 +1470,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
> > return 0;
> > }
> >
> > -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
> > +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
> > {
> > u32 reg = kvm_apic_get_reg(apic, lvt_type);
> > int vector, mode, trig_mode;
> > @@ -1470,8 +1479,10 @@ void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
> > vector = reg & APIC_VECTOR_MASK;
> > mode = reg & APIC_MODE_MASK;
> > trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
> > - __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL);
> > + return __apic_accept_irq(apic, mode, vector, 1, trig_mode,
> > + NULL);
> > }
> > + return 0;
> > }
> >
> > void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu)
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 61a73a0..c730ac9 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -57,9 +57,9 @@ void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
> > void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
> > int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
> > int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
> > -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> > - unsigned long *dest_map);
> > -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
> > +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> > + unsigned long *dest_map);
> > +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
> >
> > bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> > struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map);
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index ef1817b..e2e6b44 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -91,8 +91,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > if (!kvm_is_dm_lowest_prio(irq)) {
> > if (r < 0)
> > r = 0;
> > - kvm_apic_set_irq(vcpu, irq, dest_map);
> > - r++;
> > + r += kvm_apic_set_irq(vcpu, irq, dest_map);
> > } else if (kvm_lapic_enabled(vcpu)) {
> > if (!lowest)
> > lowest = vcpu;
> > @@ -101,10 +100,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > }
> > }
> >
> > - if (lowest) {
> > - kvm_apic_set_irq(lowest, irq, dest_map);
> > - r = 1;
> > - }
> > + if (lowest)
> > + r = kvm_apic_set_irq(lowest, irq, dest_map);
> >
> > return r;
> > }
> > --
> > Gleb.
> >
>
>
--
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
