From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Hildenbrand Subject: Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry() Date: Tue, 7 Mar 2017 10:55:54 +0100 Message-ID: <91851e4b-b028-ab5f-302b-c8b8b7e9e5d0@redhat.com> References: <20170306131815.12033-1-david@redhat.com> <20170306131815.12033-3-david@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: rkrcmar@redhat.com To: Paolo Bonzini , kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38436 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754922AbdCGKMr (ORCPT ); Tue, 7 Mar 2017 05:12:47 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 F232681226 for ; Tue, 7 Mar 2017 09:55:56 +0000 (UTC) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Am 06.03.2017 um 19:08 schrieb Paolo Bonzini: > > > On 06/03/2017 14:17, David Hildenbrand wrote: >> KVM_IRQCHIP_NONE, >> + KVM_IRQCHIP_KERNEL_INIT, /* KVM_CREATE_IRQCHIP in progress */ > > Maybe KVM_IRQCHIP_INIT_IN_PROGRESS? I tried to make it short but I agree, that is more self explaining. > >> - 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? > >> /* 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? > > 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. > Paolo > Thanks! David