From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry() Date: Tue, 7 Mar 2017 11:53:07 +0100 Message-ID: References: <20170306131815.12033-1-david@redhat.com> <20170306131815.12033-3-david@redhat.com> <91851e4b-b028-ab5f-302b-c8b8b7e9e5d0@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: rkrcmar@redhat.com To: David Hildenbrand , kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51094 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbdCGKyJ (ORCPT ); Tue, 7 Mar 2017 05:54:09 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7A25D64A69 for ; Tue, 7 Mar 2017 10:53:11 +0000 (UTC) In-Reply-To: <91851e4b-b028-ab5f-302b-c8b8b7e9e5d0@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/03/2017 10:55, David Hildenbrand wrote: > Am 06.03.2017 um 19:08 schrieb Paolo Bonzini: >>> - bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE; >>> + bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL || >>> + kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT; >> >> I suspect that if you phrase it the other way round (!= NONE && != >> KERNEL_INIT) you'll get infinitesimally better code, because it can be >> compiled to an unsigned comparison with 1. > > However, adding new modes can silently make this check wrong (e.g. > grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do > you think the optimization is worth it? I don't think we want to add new modes. >> >>> /* Matches with wmb after initializing kvm->irq_routing. */ >>> smp_rmb(); >>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c >>> index b96d389..4e4a67a 100644 >>> --- a/arch/x86/kvm/irq_comm.c >>> +++ b/arch/x86/kvm/irq_comm.c >>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm, >>> >>> switch (ue->type) { >>> case KVM_IRQ_ROUTING_IRQCHIP: >>> + if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm)) >>> + goto out; >>> delta = 0; >> >> This can be irqchip_in_kernel, after which irqchip_kernel_init can be >> removed. > > irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT, > which is not what we want here, or am I missing something? Hmm, perhaps we can split the checks to rule out KVM_IRQCHIP_NONE outside the switch, and KVM_IRQCHIP_SPLIT here? >> Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT? > > a) checking against KVM_IRQCHIP_KERNEL_INIT shouldn't be necessary due > to the kvm->lock (this code path will never see KVM_IRQCHIP_KERNEL_INIT). > > b) setting KVM_IRQCHIP_KERNEL_INIT could be done. We only initialize an > empty irq routing. But also that should never be allowed to set up > routings targeted at pic/ioapic. However that would in its current form > never happen. You're right, it'd be just a matter of keeping the code similar between the two cases. You can try it and decide when you post the next version. Thanks! Paolo