On Sun, Apr 28, 2013 at 01:27:00PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kis...@siemens.com>
>
> Since the arrival of posted interrupt support we can no longer guarantee
> that coalesced IRQs are always reported to the IRQ source. Moreover,
> accumulated APIC timer events could cause a busy loop when a VCPU should
> rather be halted. The consensus is to remove coalesced tracking from the
> LAPIC.
>
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> ---
>
> Not sure, though, if this is going too far or not far enough /wrt
> removing return values from affected functions.
>
> arch/x86/kvm/lapic.c | 62 ++++++++++++++++++-------------------------------
> arch/x86/kvm/lapic.h | 9 ++++---
> virt/kvm/ioapic.c | 23 +++++++++---------
> virt/kvm/ioapic.h | 5 ++-
> virt/kvm/irq_comm.c | 31 +++++++++++-------------
> 5 files changed, 56 insertions(+), 74 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index e29883c..7cfd35c 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 int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> - int vector, int level, int trig_mode,
> - unsigned long *dest_map);
> +static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> + int vector, int level, int trig_mode,
> + unsigned long *dest_map);
>
> -int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> - unsigned long *dest_map)
> +void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> + unsigned long *dest_map)
> {
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> - return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
> - irq->level, irq->trig_mode, dest_map);
> + __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)
> @@ -597,7 +597,8 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct
> kvm_lapic *source,
> }
>
> 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)
> + struct kvm_lapic_irq *irq,
> + unsigned long *dest_map)
> {
> struct kvm_apic_map *map;
> unsigned long bitmap = 1;
> @@ -605,10 +606,8 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm,
> struct kvm_lapic *src,
> int i;
> bool ret = false;
>
> - *r = -1;
> -
> if (irq->shorthand == APIC_DEST_SELF) {
> - *r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
> + kvm_apic_set_irq(src->vcpu, irq, dest_map);
> return true;
> }
>
> @@ -651,9 +650,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm,
> struct kvm_lapic *src,
> for_each_set_bit(i, &bitmap, 16) {
> if (!dst[i])
> continue;
> - if (*r < 0)
> - *r = 0;
> - *r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
> + kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
> }
>
> ret = true;
> @@ -662,15 +659,11 @@ out:
> return ret;
> }
>
> -/*
> - * 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)
> +/* 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)
> {
> - int result = 0;
> struct kvm_vcpu *vcpu = apic->vcpu;
>
> switch (delivery_mode) {
> @@ -684,13 +677,10 @@ static int __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) {
> - result = 1;
> + if (kvm_x86_ops->deliver_posted_interrupt)
> kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> - } else {
> - result = !apic_test_and_set_irr(vector, apic);
> -
> - if (!result) {
> + else {
> + if (apic_test_and_set_irr(vector, apic)) {
> if (trig_mode)
> apic_debug("level trig mode repeatedly "
> "for vector %d", vector);
> @@ -702,7 +692,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
> delivery_mode,
> }
> out:
> trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
> - trig_mode, vector, !result);
> + trig_mode, vector, false);
> break;
>
> case APIC_DM_REMRD:
> @@ -714,14 +704,12 @@ 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
> @@ -738,7 +726,6 @@ 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();
> @@ -760,7 +747,6 @@ out:
> delivery_mode);
> break;
> }
> - return result;
> }
>
> int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
> @@ -1470,7 +1456,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
> return 0;
> }
>
> -int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
> +void 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;
> @@ -1479,10 +1465,8 @@ int 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;
> - return __apic_accept_irq(apic, mode, vector, 1, trig_mode,
> - NULL);
> + __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL);
> }
> - return 0;
> }
>
> void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu)
> @@ -1608,8 +1592,8 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
> return;
>
> if (atomic_read(&apic->lapic_timer.pending) > 0) {
> - if (kvm_apic_local_deliver(apic, APIC_LVTT))
> - atomic_dec(&apic->lapic_timer.pending);
> + kvm_apic_local_deliver(apic, APIC_LVTT);
> + atomic_set(&apic->lapic_timer.pending, 0);
> }
> }
>
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index c730ac9..4d1b9eb 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -57,12 +57,13 @@ 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);
> -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);
> +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);
>
> 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);
> + struct kvm_lapic_irq *irq,
> + unsigned long *dest_map);
>
> u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
> void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data);
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 2d68297..606e09b 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -50,8 +50,8 @@
> #else
> #define ioapic_debug(fmt, arg...)
> #endif
> -static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
> - bool line_status);
> +static void ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
> + bool line_status);
>
> static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
> unsigned long addr,
> @@ -172,9 +172,10 @@ static int ioapic_service(struct kvm_ioapic *ioapic,
> unsigned int idx,
> pent = &ioapic->redirtbl[idx];
>
> if (!pent->fields.mask) {
> - injected = ioapic_deliver(ioapic, idx, line_status);
> - if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> + ioapic_deliver(ioapic, idx, line_status);
> + if (pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> pent->fields.remote_irr = 1;
> + injected = 1;
> }
>
> return injected;
> @@ -282,11 +283,11 @@ static void ioapic_write_indirect(struct kvm_ioapic
> *ioapic, u32 val)
> }
> }
>
> -static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool
> line_status)
> +static void ioapic_deliver(struct kvm_ioapic *ioapic, int irq,
> + bool line_status)
> {
> union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
> struct kvm_lapic_irq irqe;
> - int ret;
>
> ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x "
> "vector=%x trig_mode=%x\n",
> @@ -304,13 +305,11 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic,
> int irq, bool line_status)
>
> if (irq == RTC_GSI && line_status) {
> BUG_ON(ioapic->rtc_status.pending_eoi != 0);
> - ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> - ioapic->rtc_status.dest_map);
> - ioapic->rtc_status.pending_eoi = ret;
> + kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> + ioapic->rtc_status.dest_map);
> + ioapic->rtc_status.pending_eoi++;
kvm_irq_delivery_to_apic() can deliver to more then one vcpu, so
pending_eoi should be incremented accordingly. It means that we cannot
drop kvm_irq_delivery_to_apic() return value.
> } else
> - ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
> -
> - return ret;
> + kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
> }
>
> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 615d8c9..639fe9c 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -92,8 +92,9 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq,
> int irq_source_id,
> int level, bool line_status);
> void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> -int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> - struct kvm_lapic_irq *irq, unsigned long *dest_map);
> +void kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> + struct kvm_lapic_irq *irq,
> + unsigned long *dest_map);
> int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> void kvm_vcpu_request_scan_ioapic(struct kvm *kvm);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 25ab480..58d4538 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -65,10 +65,11 @@ inline static bool kvm_is_dm_lowest_prio(struct
> kvm_lapic_irq *irq)
> #endif
> }
>
> -int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> - struct kvm_lapic_irq *irq, unsigned long *dest_map)
> +void kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> + struct kvm_lapic_irq *irq,
> + unsigned long *dest_map)
> {
> - int i, r = -1;
> + int i;
> struct kvm_vcpu *vcpu, *lowest = NULL;
>
> if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
> @@ -77,8 +78,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
> kvm_lapic *src,
> irq->delivery_mode = APIC_DM_FIXED;
> }
>
> - if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, &r, dest_map))
> - return r;
> + if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, dest_map))
> + return;
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> if (!kvm_apic_present(vcpu))
> @@ -88,11 +89,9 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
> kvm_lapic *src,
> irq->dest_id, irq->dest_mode))
> continue;
>
> - if (!kvm_is_dm_lowest_prio(irq)) {
> - if (r < 0)
> - r = 0;
> - r += kvm_apic_set_irq(vcpu, irq, dest_map);
> - } else if (kvm_lapic_enabled(vcpu)) {
> + if (!kvm_is_dm_lowest_prio(irq))
> + kvm_apic_set_irq(vcpu, irq, dest_map);
> + else if (kvm_lapic_enabled(vcpu)) {
> if (!lowest)
> lowest = vcpu;
> else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> @@ -101,9 +100,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
> kvm_lapic *src,
> }
>
> if (lowest)
> - r = kvm_apic_set_irq(lowest, irq, dest_map);
> -
> - return r;
> + kvm_apic_set_irq(lowest, irq, dest_map);
> }
>
> static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
> @@ -133,7 +130,8 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>
> kvm_set_msi_irq(e, &irq);
>
> - return kvm_irq_delivery_to_apic(kvm, NULL, &irq, NULL);
> + kvm_irq_delivery_to_apic(kvm, NULL, &irq, NULL);
> + return 1;
> }
>
>
> @@ -141,12 +139,11 @@ static int kvm_set_msi_inatomic(struct
> kvm_kernel_irq_routing_entry *e,
> struct kvm *kvm)
> {
> struct kvm_lapic_irq irq;
> - int r;
>
> kvm_set_msi_irq(e, &irq);
>
> - if (kvm_irq_delivery_to_apic_fast(kvm, NULL, &irq, &r, NULL))
> - return r;
> + if (kvm_irq_delivery_to_apic_fast(kvm, NULL, &irq, NULL))
> + return 1;
> else
> return -EWOULDBLOCK;
> }
> --
> 1.7.3.4
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
