From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [RFC PATCH 09/11] kvm: simplify processor compat check Date: Fri, 27 Sep 2013 17:14:37 +0200 Message-ID: <5245A0DD.9070205@redhat.com> References: <1380276233-17095-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1380276233-17095-10-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <87r4ca9zmi.fsf@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Alexander Graf , Benjamin Herrenschmidt , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, " list" , Gleb Natapov To: "Aneesh Kumar K.V" Return-path: In-Reply-To: <87r4ca9zmi.fsf@linux.vnet.ibm.com> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Il 27/09/2013 15:13, Aneesh Kumar K.V ha scritto: > Alexander Graf writes: > >> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote: >> >>> From: "Aneesh Kumar K.V" >> >> Missing patch description. >> >>> Signed-off-by: Aneesh Kumar K.V >> >> I fail to see how this really simplifies things, but at the end of the >> day it's Gleb's and Paolo's call. > > will do. It avoid calling > > for_each_online_cpu(cpu) { > smp_call_function_single() > > on multiple architecture. I agree with Alex. The current code is not specially awesome; having kvm_arch_check_processor_compat take an int* disguised as a void* is a bit ugly indeed. However, the API makes sense and tells you that it is being passed as a callback (to smp_call_function_single in this case). You are making the API more complicated to use on the arch layer (because arch maintainers now have to think "do I need to check this on all online CPUs?") and making the "leaf" POWER code less legible because it still has the weird void()(void *) calling convention. If anything, you could change kvm_arch_check_processor_compat to return an int and accept no argument, and introduce a wrapper that kvm_init passes to smp_call_function_single. Paolo > We also want to make the smp call function a callback of opaque. Hence > this should be made arch specific. > > int kvm_arch_check_processor_compat(void *opaque) > { > int r,cpu; > struct kvmppc_ops *kvm_ops = (struct kvmppc_ops *)opaque; > for_each_online_cpu(cpu) { > smp_call_function_single(cpu, > kvm_ops->check_processor_compat, > &r, 1); > if (r < 0) > break; > } > return r; > } > > against > > - for_each_online_cpu(cpu) { > - smp_call_function_single(cpu, > - kvm_arch_check_processor_compat, > - &r, 1); > - if (r < 0) > - goto out_free_1; > - } > + > + r = kvm_arch_check_processor_compat(opaque); > + if (r < 0) > + goto out_free_1; > > > >> >> Which brings me to the next issue: You forgot to CC kvm@vger on your >> patch set. Gleb and Paolo don't read kvm-ppc@vger. And they shouldn't >> have to. Every kvm patch that you want review on or that should get >> applied needs to be sent to kvm@vger. If you want to tag it as PPC >> specific patch, do so by CC'ing kvm-ppc@vger. > > Will do in the next update > > -aneesh >