Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755295AbcKYRNc (ORCPT ); Fri, 25 Nov 2016 12:13:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46106 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755062AbcKYRNX (ORCPT ); Fri, 25 Nov 2016 12:13:23 -0500 Date: Fri, 25 Nov 2016 18:11:59 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() Message-ID: <20161125171159.GA6065@potion> References: <20161124163134.4630-1-rkrcmar@redhat.com> <20161124163134.4630-3-rkrcmar@redhat.com> <2093812530.1733849.1480006551335.JavaMail.zimbra@redhat.com> <20161124172104.GA17619@potion> <801075538.121891.1480064356769.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <801075538.121891.1480064356769.JavaMail.zimbra@redhat.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 25 Nov 2016 17:12:02 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2113 Lines: 72 2016-11-25 03:59-0500, Paolo Bonzini: > ----- Original Message ----- >> From: "Radim Krčmář" >> To: "Paolo Bonzini" >> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org >> Sent: Thursday, November 24, 2016 6:21:04 PM >> Subject: Re: [PATCH 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() >> >> 2016-11-24 11:55-0500, Paolo Bonzini: >> >> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it >> >> just complicated the code. >> >> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split. >> >> (Turning them into an exclusive type would be nicer.) >> > >> > Then do it. ;) >> >> It is hard to name! :) >> >> I would squash something like this if the names were ok: >> >> diff --git a/arch/x86/include/asm/kvm_host.h >> b/arch/x86/include/asm/kvm_host.h >> index 929228ec2839..726235f0e3f3 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -706,6 +706,12 @@ struct kvm_hv { >> HV_REFERENCE_TSC_PAGE tsc_ref; >> }; >> >> +enum kvm_kernel_irqchip { > > kvm_kernel_irqchip_mode? If we append the mode, what about just "kvm_irqchip_mode"? irqchip_in_kernel() tells which one is in the kernel. >> + KVM_IRQCHIP_NONE, >> + KVM_IRQCHIP_KVM, /* created in KVM_CREATE_IRQCHIP */ >> + KVM_IRQCHIP_SPLIT, /* created in KVM_CAP_SPLIT_IRQCHIP */ > > Since you pretty much asked to nitpick, I am always interested in nitpicks! > KVM_IRQCHIP_KERNEL would > match irqchip_in_kernel better. Matching the enum name prefix would be nice, so I'd rename it to enum kvm_irqchip_kernel_mode then. I'd keep them this way if we go with enum kvm_irqchip_mode. > Also, s/in/with/? :) Ok. >> +}; >> + >> struct kvm_arch { >> unsigned int n_used_mmu_pages; >> unsigned int n_requested_mmu_pages; >> @@ -778,8 +784,7 @@ struct kvm_arch { >> >> u64 disabled_quirks; >> >> - bool irqchip_kvm; >> - bool irqchip_split; >> + enum kvm_kernel_irqchip irqchip; > > irqchip_mode? Yes, thanks.