From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752507AbbCTOnK (ORCPT ); Fri, 20 Mar 2015 10:43:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50642 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752416AbbCTOnG (ORCPT ); Fri, 20 Mar 2015 10:43:06 -0400 Date: Fri, 20 Mar 2015 15:43:02 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Marcelo Tosatti Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , chao.zhou@intel.com Subject: Re: [PATCH] KVM: x86: call irq notifiers with directed EOI Message-ID: <20150320144302.GA14772@potion.brq.redhat.com> References: <1426703902-16818-1-git-send-email-rkrcmar@redhat.com> <20150319214449.GA4264@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150319214449.GA4264@amt.cnet> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2015-03-19 18:44-0300, Marcelo Tosatti: > On Wed, Mar 18, 2015 at 07:38:22PM +0100, Radim Krčmář wrote: > > kvm_ioapic_update_eoi() wasn't called if directed EOI was enabled. > > We need to do that for irq notifiers. (Like with edge interrupts.) > > > > Fix it by skipping EOI broadcast only. > > > > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=82211 > > Signed-off-by: Radim Krčmář > > --- > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > > @@ -443,7 +444,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, > > - if (trigger_mode != IOAPIC_LEVEL_TRIG) > > + if (trigger_mode != IOAPIC_LEVEL_TRIG || > > + kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) > > continue; > > Don't you have to handle kvm_ioapic_eoi_inject_work as well? It works without that: ent->fields.remote_irr == 1, thus kvm_ioapic_eoi_inject_work() will do nothing. Adding a check would be better for clarity, though. We could add the EOI register (implement IO-APIC version 0x20), because kernels are forced to do ugly hacks otherwise (switching to edge-triggered mode and back). We also clear remote_irr on a different occasion (just a write to ioreg). I'll take a closer look at the second one. > > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > > This assert can now fail? I think it can't (nothing changed), but that is how asserts should be. It checks a different variable than the condition above. ('trigger_mode' is sourced from APIC_TMR, which should correctly match 'ent->fields.trig_mode'.) The assert would be more useful before 'continue;', and modified: ASSERT(ent->fields.trig_mode == trigger_mode) Thanks for the review, I'll incorporate the your comments to v2.