From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry() Date: Tue, 7 Mar 2017 15:40:17 +0100 Message-ID: <20170307144016.GA18298@potion> 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=us-ascii Cc: David Hildenbrand , kvm@vger.kernel.org To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37588 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932367AbdCGOkU (ORCPT ); Tue, 7 Mar 2017 09:40:20 -0500 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E38BEC9D5C for ; Tue, 7 Mar 2017 14:40:20 +0000 (UTC) Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 2017-03-07 11:53+0100, Paolo Bonzini: > 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. Yes, it seems that the compiler must assume that an enum can also be a value that is not enumerated. >> 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. I would prefer to write it like: kvm->arch.irqchip_mode > KVM_IRQCHIP_KERNEL_INIT; Same assembly with simpler code. Setting KVM_IRQCHIP_KERNEL_INIT before KVM_IRQCHIP_SPLIT would make it a bit more descriprive (and would allow the check below as well). >>>> /* 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? Right, we don't want MSI or HV without LAPIC in kernel either.