Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964792AbbLWCNG (ORCPT ); Tue, 22 Dec 2015 21:13:06 -0500 Received: from mga03.intel.com ([134.134.136.65]:19670 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752871AbbLWCNE convert rfc822-to-8bit (ORCPT ); Tue, 22 Dec 2015 21:13:04 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,467,1444719600"; d="scan'208";a="867851680" From: "Wu, Feng" To: "rkrcmar@redhat.com" CC: Yang Zhang , "pbonzini@redhat.com" , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Jiang Liu (jiang.liu@linux.intel.com)" , "Wu, Feng" Subject: RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Thread-Topic: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Thread-Index: AQHRO5FzVNvWlqsXtkO3uwvyjy20qJ7UrBtA//9/AQCAAIb+oIABWmEAgACIKwD//36iAIAAhuYAgABNSoCAAO43wA== Date: Wed, 23 Dec 2015 02:12:58 +0000 Message-ID: References: <1450229853-3886-1-git-send-email-feng.wu@intel.com> <1450229853-3886-2-git-send-email-feng.wu@intel.com> <567759F2.5080809@gmail.com> <56775E9A.5030108@gmail.com> <5678F268.5070801@gmail.com> <5678F81C.5050309@gmail.com> <20151222195259.GC12725@potion.redhat.com> In-Reply-To: <20151222195259.GC12725@potion.redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4205 Lines: 102 > -----Original Message----- > From: rkrcmar@redhat.com [mailto:rkrcmar@redhat.com] > Sent: Wednesday, December 23, 2015 3:53 AM > To: Wu, Feng > Cc: Yang Zhang ; pbonzini@redhat.com; > kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Jiang Liu > (jiang.liu@linux.intel.com) > Subject: Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest- > priority interrupts > > 2015-12-22 07:19+0000, Wu, Feng: > >> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com] > >> On 2015/12/22 14:59, Wu, Feng wrote: > >> >> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com] > >> >>>>>> On 2015/12/16 9:37, Feng Wu wrote: > >> >>>>>>> + for_each_set_bit(i, &bitmap, 16) { > >> >>>>>>> + if (!dst[i] > >> >>>>>> && !kvm_lapic_enabled(dst[i]->vcpu)) { > >> >>>>>> > >> >>>>>> It should be or(||) not and (&&). > >> >>>>> > >> >>>>> Oh, you are right! My negligence! Thanks for pointing this out, Yang! > >> >>>> > >> >>>> btw, i think the kvm_lapic_enabled check is wrong here? Why need it > here? > >> >>> > >> >>> If the lapic is not enabled, I think we cannot recognize it as a candidate, > can > >> >> we? > >> >>> Maybe Radim can confirm this, Radim, what is your option? > > SDM 10.6.2.2 Logical Destination Mode: > For both configurations of logical destination mode, when combined > with lowest priority delivery mode, software is responsible for > ensuring that all of the local APICs included in or addressed by the > IPI or I/O subsystem interrupt are present and enabled to receive the > interrupt. > Radim, thanks a lot for your feedback! > The case is undefined if some targeted LAPICs weren't hardware enabled > as no interrupts can be delivered to hardware disabled LAPIC, so we can > check for hardware enabled. > > It's not obvious if "enabled to receive the interrupt" means hardware or > software enabled, but lowest priority cannot deliver NMI/INIT/..., so > checking for software enabled doesn't restrict any valid uses either. > > so ... KVM only musn't blow up when encountering this situation :) > > The current code seems correct, but redundant. Just for reference, KVM > now does: > - check for software enabled LAPIC since patch aefd18f01ee8 ("KVM: x86: > In DM_LOWEST, only deliver interrupts to vcpus with enabled LAPIC's") > - check only for hardware enabled LAPIC in the fast path, since > 1e08ec4a130e ("KVM: optimize apic interrupt delivery")) Software enabled LAPIC is also checked in patch 1e08ec4a130e ("KVM: optimize apic interrupt delivery"), however, it was removed in patch 3b5a5ffa928a3f875b0d5dd284eeb7c322e1688a. Now I am a little confused about the policy, when and where should we do the software/hardware enabled check? > > (v1 was arguable better, I pointed the need for enabled LAPIC in v1 only > from looking at one KVM function, sorry.) > > >> >> Lapic can be disable by hw or sw. Here we only need to check the hw is > >> >> enough which is already covered while injecting the interrupt into > >> >> guest. I remember we(Glab, Macelo and me) have discussed it several ago, > >> >> but i cannot find the mail thread. > >> > >> > > >> > But if the lapic is disabled by software, we cannot still inject interrupts to > >> > it, can we? > >> > >> Yes, We cannot inject the normal interrupt. But this already covered by > >> current logic and add a check here seems meaningless. Conversely, it may > >> do bad thing.. > >> > > > > Let's wait for Radim/Paolo's opinions about this. > > I'd pick whatever results in less code: this time it seems like checking > for hardware enabled LAPIC in both paths (implicitly in the fast path). > Maybe it can be done better, I haven't given it much thought. > > We should revert aefd18f01ee8 at the same time, so our PI/non-PI slow > paths won't diverge -- I hope it wasn't fixing a bug :) >From the change log, It seems to me this patch was fixing a bug. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/