Re: [RFC PATCH 09/11] kvm: simplify processor compat check

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
To: Gleb Natapov <gleb@redhat.com>
Cc: "<kvm@vger.kernel.org> list" <kvm@vger.kernel.org>,
	Alexander Graf <agraf@suse.de>,
	kvm-ppc@vger.kernel.org, Paul Mackerras <paulus@samba.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [RFC PATCH 09/11] kvm: simplify processor compat check
Date: Sun, 29 Sep 2013 20:35:16 +0530	[thread overview]
Message-ID: <87siwnbrdf.fsf@linux.vnet.ibm.com> (raw)
In-Reply-To: <20130929085800.GO17294@redhat.com>

Gleb Natapov <gleb@redhat.com> writes:

> On Sat, Sep 28, 2013 at 09:06:47PM +0530, Aneesh Kumar K.V wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > Il 27/09/2013 15:13, Aneesh Kumar K.V ha scritto:
>> >> Alexander Graf <agraf@suse.de> writes:
>> >> 
>> >>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
>> >>>
>> >>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> >>>
>> >>> Missing patch description.
>> >>>
>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> >>>
>> >>> 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).
>> 
>> But whether to check on all cpus or not is arch dependent right?.
>> IIUC only x86 and ppc64 need to do that. Also on ppc64 it really
>> depends on whether HV or PR. We need to check on all cpus only if it is
>> HV. 
>> 
>> >
>> > 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.
>> >
>> 
>> IIUC what we wanted to check is to find out whether kvm can run on this
>> system. That is really an arch specific check. So for core kvm the call
>> should be a simple 
>> 
>> if (kvm_arch_check_process_compat() < 0)
>>         error;
> We have that already, just return error from kvm_arch_hardware_setup. This
> is specific processor compatibility check and you are arguing that the
> processor check should be part of kvm_arch_hardware_setup().

What about the success case ?. ie, on arch like arm we do

void kvm_arch_check_processor_compat(void *rtn)
{
	*(int *)rtn = 0;
}

for_each_online_cpu(cpu) {
	smp_call_function_single(cpu,
			kvm_arch_check_processor_compat,
			&r, 1);
	if (r < 0)
		goto out_free_1;
}

There is no need to do that for loop for arm. 

The only reason I wanted this patch in the series is to make
kvm_arch_check_processor_compat take additional argument opaque. 
I am dropping that requirement in the last patch. Considering
that we have objection to this one, I will drop this patch in
the next posting by rearranging the patches.

-aneesh