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

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

[Alexander Graf]

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.

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.


Alex

> ---
> arch/arm/kvm/arm.c                 |  4 ++--
> arch/ia64/kvm/kvm-ia64.c           |  4 ++--
> arch/mips/kvm/kvm_mips.c           |  6 ++----
> arch/powerpc/include/asm/kvm_ppc.h |  2 +-
> arch/powerpc/kvm/44x.c             |  2 +-
> arch/powerpc/kvm/book3s.c          | 15 ++++++++++++---
> arch/powerpc/kvm/book3s_hv.c       |  9 ++++++---
> arch/powerpc/kvm/book3s_pr.c       |  5 +++--
> arch/powerpc/kvm/e500.c            |  2 +-
> arch/powerpc/kvm/e500mc.c          |  2 +-
> arch/powerpc/kvm/powerpc.c         |  5 -----
> arch/s390/kvm/kvm-s390.c           |  3 ++-
> arch/x86/kvm/x86.c                 | 13 +++++++++++--
> include/linux/kvm_host.h           |  2 +-
> virt/kvm/kvm_main.c                | 14 +++++---------
> 15 files changed, 50 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9c697db..cccb121 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -109,9 +109,9 @@ void kvm_arch_hardware_unsetup(void)
> {
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> -	*(int *)rtn = 0;
> +	return 0;
> }
> 
> void kvm_arch_sync_events(struct kvm *kvm)
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index bdfd878..065942c 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -185,9 +185,9 @@ void kvm_arch_hardware_disable(void *garbage)
> 	ia64_ptr_entry(0x3, slot);
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> -	*(int *)rtn = 0;
> +	return 0;
> }
> 
> int kvm_dev_ioctl_check_extension(long ext)
> diff --git a/arch/mips/kvm/kvm_mips.c b/arch/mips/kvm/kvm_mips.c
> index a7b0445..4512739 100644
> --- a/arch/mips/kvm/kvm_mips.c
> +++ b/arch/mips/kvm/kvm_mips.c
> @@ -97,11 +97,9 @@ void kvm_arch_hardware_unsetup(void)
> {
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> -	int *r = (int *)rtn;
> -	*r = 0;
> -	return;
> +	return 0;
> }
> 
> static void kvm_mips_init_tlbs(struct kvm *kvm)
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 58e732f..592501b 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -204,7 +204,7 @@ struct kvmppc_ops {
> 			      unsigned long npages);
> 	int (*init_vm)(struct kvm *kvm);
> 	void (*destroy_vm)(struct kvm *kvm);
> -	int (*check_processor_compat)(void);
> +	void (*check_processor_compat)(void *r);
> 	int (*get_smmu_info)(struct kvm *kvm, struct kvm_ppc_smmu_info *info);
> 	int (*emulate_op)(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 			  unsigned int inst, int *advance);
> diff --git a/arch/powerpc/kvm/44x.c b/arch/powerpc/kvm/44x.c
> index 2f5c6b6..a1f4e60 100644
> --- a/arch/powerpc/kvm/44x.c
> +++ b/arch/powerpc/kvm/44x.c
> @@ -43,7 +43,7 @@ void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> 	kvmppc_booke_vcpu_put(vcpu);
> }
> 
> -int kvmppc_core_check_processor_compat(void)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> 	int r;
> 
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index ca617e1..485a6ff 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -827,9 +827,18 @@ void kvmppc_core_destroy_vm(struct kvm *kvm)
> #endif
> }
> 
> -int kvmppc_core_check_processor_compat(void)
> -{
> -	return kvmppc_ops->check_processor_compat();
> +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;
> }
> 
> EXPORT_SYMBOL_GPL(kvm_get_dirty_log);
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index ff57be8..4322db4 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1980,11 +1980,14 @@ static int kvmppc_core_emulate_mfspr_hv(struct kvm_vcpu *vcpu, int sprn,
> 	return EMULATE_FAIL;
> }
> 
> -static int kvmppc_core_check_processor_compat_hv(void)
> +
> +static void kvmppc_core_check_processor_compat_hv(void *r)
> {
> 	if (!cpu_has_feature(CPU_FTR_HVMODE))
> -		return -EIO;
> -	return 0;
> +		*(int *)r = -EIO;
> +	else
> +		*(int *)r = 0;
> +	return;
> }
> 
> static long kvm_arch_vm_ioctl_hv(struct file *filp,
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index df48d89..127b961 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1490,10 +1490,11 @@ static void kvmppc_core_destroy_vm_pr(struct kvm *kvm)
> 	enable_relon_interrupts(kvm);
> }
> 
> -static int kvmppc_core_check_processor_compat_pr(void)
> +static void kvmppc_core_check_processor_compat_pr(void *r)
> {
> 	/* we are always compatible */
> -	return 0;
> +	*(int *)r = 0;
> +	return;
> }
> 
> static long kvm_arch_vm_ioctl_pr(struct file *filp,
> diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
> index ce6b73c..0681cb1 100644
> --- a/arch/powerpc/kvm/e500.c
> +++ b/arch/powerpc/kvm/e500.c
> @@ -323,7 +323,7 @@ void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> 	kvmppc_booke_vcpu_put(vcpu);
> }
> 
> -int kvmppc_core_check_processor_compat(void)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> 	int r;
> 
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index 19c8379..48b3ba5 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -169,7 +169,7 @@ void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> 	kvmppc_booke_vcpu_put(vcpu);
> }
> 
> -int kvmppc_core_check_processor_compat(void)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> 	int r;
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 3019edc..1404f4d 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -264,11 +264,6 @@ void kvm_arch_hardware_unsetup(void)
> {
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> -{
> -	*(int *)rtn = kvmppc_core_check_processor_compat();
> -}
> -
> int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> {
> 	if (type)
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 776dafe..01d4000 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -119,8 +119,9 @@ void kvm_arch_hardware_unsetup(void)
> 	gmap_unregister_ipte_notifier(&gmap_notifier);
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> +	return 0;
> }
> 
> int kvm_arch_init(void *opaque)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5ca72a..53c8308 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6873,9 +6873,18 @@ void kvm_arch_hardware_unsetup(void)
> 	kvm_x86_ops->hardware_unsetup();
> }
> 
> -void kvm_arch_check_processor_compat(void *rtn)
> +int kvm_arch_check_processor_compat(void *opaque)
> {
> -	kvm_x86_ops->check_processor_compatibility(rtn);
> +	int r,cpu;
> +
> +	for_each_online_cpu(cpu) {
> +		smp_call_function_single(cpu,
> +			 kvm_x86_ops->check_processor_compatibility,
> +			 &r, 1);
> +		if (r < 0)
> +			break;
> +	}
> +	return r;
> }
> 
> bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu)
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ca645a0..459b359 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -649,7 +649,7 @@ int kvm_arch_hardware_enable(void *garbage);
> void kvm_arch_hardware_disable(void *garbage);
> int kvm_arch_hardware_setup(void);
> void kvm_arch_hardware_unsetup(void);
> -void kvm_arch_check_processor_compat(void *rtn);
> +int kvm_arch_check_processor_compat(void *opaque);
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
> int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 66df1d2..0594b22 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3166,10 +3166,9 @@ static void kvm_sched_out(struct preempt_notifier *pn,
> }
> 
> int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> -		  struct module *module)
> +	     struct module *module)
> {
> 	int r;
> -	int cpu;
> 
> 	r = kvm_arch_init(opaque);
> 	if (r)
> @@ -3195,13 +3194,10 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> 	if (r < 0)
> 		goto out_free_0a;
> 
> -	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;
> 
> 	r = register_cpu_notifier(&kvm_cpu_notifier);
> 	if (r)
> -- 
> 1.8.1.2
> 

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

[Aneesh Kumar K.V]

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.

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

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

[Paolo Bonzini]

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).

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
> 

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

[Aneesh Kumar K.V]

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;

Now how each arch figure out whether kvm can run on this system should
be arch specific. For x86 we do need to check all the cpus. On ppc64 for
HV we need to. For other archs we always allow kvm. 


> 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.

What i am suggesting in the patch is to avoid calling
smp_call_function_single from kvm_init and let arch decide whether to
check on all cpus or not.

-aneesh

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

[Gleb Natapov]

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().

> 
> Now how each arch figure out whether kvm can run on this system should
> be arch specific. For x86 we do need to check all the cpus. On ppc64 for
> HV we need to. For other archs we always allow kvm. 
> 
This is really a sanity check. Theoretically on x86 we also should
not need to check all cpus since SMP configuration with different cpu
models is not supported by the architecture (AFAIK), but bugs happen
(BIOS bugs may cause difference in capabilities for instance). So some
arches opted out from this sanity check for now and this is their choice,
but the code makes it explicit what are we checking here.

> 
> > 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.
> 
> What i am suggesting in the patch is to avoid calling
> smp_call_function_single from kvm_init and let arch decide whether to
> check on all cpus or not.
> 
> -aneesh

--
			Gleb.

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

[Aneesh Kumar K.V]

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

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

[Gleb Natapov]

On Sun, Sep 29, 2013 at 08:35:16PM +0530, Aneesh Kumar K.V wrote:
> 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) {
As I said they opted out from doing the check. They may reconsider after
first bad HW will be discovered.

> 	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. 
It's done once during module initialisation. Why is this a big deal?

--
			Gleb.

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Alexander Graf]

On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:

> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> 
>> Hi All,
>> 
>> This patch series support enabling HV and PR KVM together in the same kernel. We
>> extend machine property with new property "kvm_type". A value of 1 will force HV
>> KVM and 2 PR KVM. The default value is 0 which will select the fastest KVM mode.
>> ie, HV if that is supported otherwise PR.
>> 
>> With Qemu command line having
>> 
>> -machine pseries,accel=kvm,kvm_type=1
>> 
>> [root@llmp24l02 qemu]# bash ../qemu
>> failed to initialize KVM: Invalid argument
>> [root@llmp24l02 qemu]# modprobe kvm-pr
>> [root@llmp24l02 qemu]# bash ../qemu
>> failed to initialize KVM: Invalid argument
>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>> [root@llmp24l02 qemu]# bash ../qemu
>> 
>> now with
>> 
>> -machine pseries,accel=kvm,kvm_type=2
>> 
>> [root@llmp24l02 qemu]# rmmod kvm-pr
>> [root@llmp24l02 qemu]# bash ../qemu
>> failed to initialize KVM: Invalid argument
>> [root@llmp24l02 qemu]#
>> [root@llmp24l02 qemu]# modprobe kvm-pr
>> [root@llmp24l02 qemu]# bash ../qemu
>> 
>> if don't specify kvm_type machine property, it will take a default value 0,
>> which means fastest supported.
> 
> Related qemu patch
> 
> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Date:   Mon Sep 23 12:28:37 2013 +0530
> 
>    kvm: Add a new machine property kvm_type
> 
>    Targets like ppc64 support different type of KVM, one which use
>    hypervisor mode and the other which doesn't. Add a new machine
>    property kvm_type that helps in selecting the respective ones
> 
>    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

This really is too early, as we can't possibly run in HV mode for non-pseries machines, so the interpretation (or at least sanity checking) of what values are reasonable should occur in the machine. That's why it's a variable in the "machine opts".

Also, users don't want to say type=0. They want to say type=PR or type=HV or type=HV,PR. In fact, can't you make this a property of -accel? Then it's truly accel specific and everything should be well.


Alex

> 
> diff --git a/kvm-all.c b/kvm-all.c
> index b87215c..a061eda 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1350,7 +1350,7 @@ int kvm_init(void)
>     KVMState *s;
>     const KVMCapabilityInfo *missing_cap;
>     int ret;
> -    int i;
> +    int i, kvm_type;
>     int max_vcpus;
> 
>     s = g_malloc0(sizeof(KVMState));
> @@ -1407,7 +1407,8 @@ int kvm_init(void)
>         goto err;
>     }
> 
> -    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> +    kvm_type = qemu_opt_get_number(qemu_get_machine_opts(), "kvm_type", 0);
> +    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, kvm_type);
>     if (s->vmfd < 0) {
> #ifdef TARGET_S390X
>         fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
> diff --git a/vl.c b/vl.c
> index 4e709d5..4374b17 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -427,7 +427,12 @@ static QemuOptsList qemu_machine_opts = {
>             .name = "usb",
>             .type = QEMU_OPT_BOOL,
>             .help = "Set on/off to enable/disable usb",
> +        },{
> +            .name = "kvm_type",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Set to kvm type to be used in create vm ioctl",
>         },
> +
>         { /* End of list */ }
>     },
> };
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:
>
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>> 
>>> Hi All,
>>> 
>>> This patch series support enabling HV and PR KVM together in the same kernel. We
>>> extend machine property with new property "kvm_type". A value of 1 will force HV
>>> KVM and 2 PR KVM. The default value is 0 which will select the fastest KVM mode.
>>> ie, HV if that is supported otherwise PR.
>>> 
>>> With Qemu command line having
>>> 
>>> -machine pseries,accel=kvm,kvm_type=1
>>> 
>>> [root@llmp24l02 qemu]# bash ../qemu
>>> failed to initialize KVM: Invalid argument
>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>> [root@llmp24l02 qemu]# bash ../qemu
>>> failed to initialize KVM: Invalid argument
>>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>>> [root@llmp24l02 qemu]# bash ../qemu
>>> 
>>> now with
>>> 
>>> -machine pseries,accel=kvm,kvm_type=2
>>> 
>>> [root@llmp24l02 qemu]# rmmod kvm-pr
>>> [root@llmp24l02 qemu]# bash ../qemu
>>> failed to initialize KVM: Invalid argument
>>> [root@llmp24l02 qemu]#
>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>> [root@llmp24l02 qemu]# bash ../qemu
>>> 
>>> if don't specify kvm_type machine property, it will take a default value 0,
>>> which means fastest supported.
>> 
>> Related qemu patch
>> 
>> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
>> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Date:   Mon Sep 23 12:28:37 2013 +0530
>> 
>>    kvm: Add a new machine property kvm_type
>> 
>>    Targets like ppc64 support different type of KVM, one which use
>>    hypervisor mode and the other which doesn't. Add a new machine
>>    property kvm_type that helps in selecting the respective ones
>> 
>>    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
> This really is too early, as we can't possibly run in HV mode for
> non-pseries machines, so the interpretation (or at least sanity
> checking) of what values are reasonable should occur in the
> machine. That's why it's a variable in the "machine opts".

With the current code CREATE_VM will fail, because we won't have
kvm-hv.ko loaded and trying to create a vm with type 1 will fail. 
Now the challenge related to moving that to machine_init or later is, we
depend on HV or PR callback early in CREATE_VM. With the changes we have

int kvmppc_core_init_vm(struct kvm *kvm)
{

#ifdef CONFIG_PPC64
	INIT_LIST_HEAD(&kvm->arch.spapr_tce_tables);
	INIT_LIST_HEAD(&kvm->arch.rtas_tokens);
#endif

	return kvm->arch.kvm_ops->init_vm(kvm);
}

Also the mmu notifier callback do end up calling kvm_unmap_hva etc which
are all HV/PR dependent. 



>
> Also, users don't want to say type=0. They want to say type=PR or
> type=HV or type=HV,PR. In fact, can't you make this a property of
> -accel? Then it's truly accel specific and everything should be well.

If we are doing this as machine property, we can't specify string,
because "HV"/"PR" are all powerpc dependent, so parsing that is not
possible in kvm_init in qemu. But, yes ideally it would be nice to be
able to speicy the type using string. I thought accel is a machine
property, hence was not sure whether I can have additional properties
against that. I was using it as below.

 -machine pseries,accel=kvm,kvm_type=1

will look into more details to check whether this can be accel property.

-aneesh

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Alexander Graf]

On 09/30/2013 03:09 PM, Aneesh Kumar K.V wrote:
> Alexander Graf<agraf@suse.de>  writes:
>
>> On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:
>>
>>> "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>  writes:
>>>
>>>> Hi All,
>>>>
>>>> This patch series support enabling HV and PR KVM together in the same kernel. We
>>>> extend machine property with new property "kvm_type". A value of 1 will force HV
>>>> KVM and 2 PR KVM. The default value is 0 which will select the fastest KVM mode.
>>>> ie, HV if that is supported otherwise PR.
>>>>
>>>> With Qemu command line having
>>>>
>>>> -machine pseries,accel=kvm,kvm_type=1
>>>>
>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>> failed to initialize KVM: Invalid argument
>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>> failed to initialize KVM: Invalid argument
>>>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>
>>>> now with
>>>>
>>>> -machine pseries,accel=kvm,kvm_type=2
>>>>
>>>> [root@llmp24l02 qemu]# rmmod kvm-pr
>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>> failed to initialize KVM: Invalid argument
>>>> [root@llmp24l02 qemu]#
>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>
>>>> if don't specify kvm_type machine property, it will take a default value 0,
>>>> which means fastest supported.
>>> Related qemu patch
>>>
>>> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
>>> Author: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>> Date:   Mon Sep 23 12:28:37 2013 +0530
>>>
>>>     kvm: Add a new machine property kvm_type
>>>
>>>     Targets like ppc64 support different type of KVM, one which use
>>>     hypervisor mode and the other which doesn't. Add a new machine
>>>     property kvm_type that helps in selecting the respective ones
>>>
>>>     Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>> This really is too early, as we can't possibly run in HV mode for
>> non-pseries machines, so the interpretation (or at least sanity
>> checking) of what values are reasonable should occur in the
>> machine. That's why it's a variable in the "machine opts".
> With the current code CREATE_VM will fail, because we won't have
> kvm-hv.ko loaded and trying to create a vm with type 1 will fail.
> Now the challenge related to moving that to machine_init or later is, we
> depend on HV or PR callback early in CREATE_VM. With the changes we have
>
> int kvmppc_core_init_vm(struct kvm *kvm)
> {
>
> #ifdef CONFIG_PPC64
> 	INIT_LIST_HEAD(&kvm->arch.spapr_tce_tables);
> 	INIT_LIST_HEAD(&kvm->arch.rtas_tokens);
> #endif
>
> 	return kvm->arch.kvm_ops->init_vm(kvm);
> }
>
> Also the mmu notifier callback do end up calling kvm_unmap_hva etc which
> are all HV/PR dependent.

Yes, so we should verify in the machine models that we're runnable with 
the currently selected type at least, to give the user a sensible error 
message.

>
>
>
>> Also, users don't want to say type=0. They want to say type=PR or
>> type=HV or type=HV,PR. In fact, can't you make this a property of
>> -accel? Then it's truly accel specific and everything should be well.
> If we are doing this as machine property, we can't specify string,
> because "HV"/"PR" are all powerpc dependent, so parsing that is not
> possible in kvm_init in qemu. But, yes ideally it would be nice to be

Well, we could do the "name to integer" conversion in an arch specific 
function, no?

> able to speicy the type using string. I thought accel is a machine
> property, hence was not sure whether I can have additional properties
> against that. I was using it as below.
>
>   -machine pseries,accel=kvm,kvm_type=1
>
> will look into more details to check whether this can be accel property.

Great :).


Alex

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 09/30/2013 03:09 PM, Aneesh Kumar K.V wrote:
>> Alexander Graf<agraf@suse.de>  writes:
>>
>>> On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:
>>>
>>>> "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>  writes:
>>>>
>>>>> Hi All,
>>>>>
>>>>> This patch series support enabling HV and PR KVM together in the same kernel. We
>>>>> extend machine property with new property "kvm_type". A value of 1 will force HV
>>>>> KVM and 2 PR KVM. The default value is 0 which will select the fastest KVM mode.
>>>>> ie, HV if that is supported otherwise PR.
>>>>>
>>>>> With Qemu command line having
>>>>>
>>>>> -machine pseries,accel=kvm,kvm_type=1
>>>>>
>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>> failed to initialize KVM: Invalid argument
>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>> failed to initialize KVM: Invalid argument
>>>>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>
>>>>> now with
>>>>>
>>>>> -machine pseries,accel=kvm,kvm_type=2
>>>>>
>>>>> [root@llmp24l02 qemu]# rmmod kvm-pr
>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>> failed to initialize KVM: Invalid argument
>>>>> [root@llmp24l02 qemu]#
>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>
>>>>> if don't specify kvm_type machine property, it will take a default value 0,
>>>>> which means fastest supported.
>>>> Related qemu patch
>>>>
>>>> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
>>>> Author: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>>> Date:   Mon Sep 23 12:28:37 2013 +0530
>>>>
>>>>     kvm: Add a new machine property kvm_type
>>>>
>>>>     Targets like ppc64 support different type of KVM, one which use
>>>>     hypervisor mode and the other which doesn't. Add a new machine
>>>>     property kvm_type that helps in selecting the respective ones
>>>>
>>>>     Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>> This really is too early, as we can't possibly run in HV mode for
>>> non-pseries machines, so the interpretation (or at least sanity
>>> checking) of what values are reasonable should occur in the
>>> machine. That's why it's a variable in the "machine opts".
>> With the current code CREATE_VM will fail, because we won't have
>> kvm-hv.ko loaded and trying to create a vm with type 1 will fail.
>> Now the challenge related to moving that to machine_init or later is, we
>> depend on HV or PR callback early in CREATE_VM. With the changes we have
>>
>> int kvmppc_core_init_vm(struct kvm *kvm)
>> {
>>
>> #ifdef CONFIG_PPC64
>> 	INIT_LIST_HEAD(&kvm->arch.spapr_tce_tables);
>> 	INIT_LIST_HEAD(&kvm->arch.rtas_tokens);
>> #endif
>>
>> 	return kvm->arch.kvm_ops->init_vm(kvm);
>> }
>>
>> Also the mmu notifier callback do end up calling kvm_unmap_hva etc which
>> are all HV/PR dependent.
>
> Yes, so we should verify in the machine models that we're runnable with 
> the currently selected type at least, to give the user a sensible error 
> message.

Something like the below

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 004184d..7d59ac1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1337,6 +1337,21 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     assert(spapr->fdt_skel != NULL);
 }
 
+static int spapr_get_vm_type(const char *vm_type)
+{
+    if (!vm_type)
+        return 0;
+
+    if (!strcmp(vm_type, "HV"))
+        return 1;
+
+    if (!strcmp(vm_type, "PR"))
+        return 2;
+
+    hw_error("qemu: unknown kvm_type specified '%s'", vm_type);
+    exit(1);
+}
+
 static QEMUMachine spapr_machine = {
     .name = "pseries",
     .desc = "pSeries Logical Partition (PAPR compliant)",
@@ -1347,6 +1362,7 @@ static QEMUMachine spapr_machine = {
     .max_cpus = MAX_CPUS,
     .no_parallel = 1,
     .default_boot_order = NULL,
+    .get_vm_type = spapr_get_vm_type,
 };
 
 static void spapr_machine_init(void)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5a7ae9f..2130488 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -21,6 +21,8 @@ typedef void QEMUMachineResetFunc(void);
 
 typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
 
+typedef int QEMUMachineGetVmTypeFunc(const char *arg);
+
 typedef struct QEMUMachine {
     const char *name;
     const char *alias;
@@ -28,6 +30,7 @@ typedef struct QEMUMachine {
     QEMUMachineInitFunc *init;
     QEMUMachineResetFunc *reset;
     QEMUMachineHotAddCPUFunc *hot_add_cpu;
+    QEMUMachineGetVmTypeFunc *get_vm_type;
     BlockInterfaceType block_default_type;
     int max_cpus;
     unsigned int no_serial:1,
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index e1f88bf..acc3d74 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -36,7 +36,8 @@ void xen_cmos_set_s3_resume(void *opaque, int irq, int level);
 
 qemu_irq *xen_interrupt_controller_init(void);
 
-int xen_init(void);
+typedef struct QEMUMachine QEMUMachine;
+int xen_init(QEMUMachine *machine);
 int xen_hvm_init(MemoryRegion **ram_memory);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 9bbe3db..f25caec 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -142,8 +142,8 @@ typedef struct KVMState KVMState;
 extern KVMState *kvm_state;
 
 /* external API */
-
-int kvm_init(void);
+typedef struct QEMUMachine QEMUMachine;
+int kvm_init(QEMUMachine *machine);
 
 int kvm_has_sync_mmu(void);
 int kvm_has_vcpu_events(void);
diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
index 9a0c6b3..d71343d 100644
--- a/include/sysemu/qtest.h
+++ b/include/sysemu/qtest.h
@@ -31,7 +31,8 @@ static inline int qtest_available(void)
     return 1;
 }
 
-int qtest_init(void);
+typedef struct QEMUMachine QEMUMachine;
+int qtest_init(QEMUMachine *machine);
 #else
 static inline bool qtest_enabled(void)
 {
@@ -43,7 +44,7 @@ static inline int qtest_available(void)
     return 0;
 }
 
-static inline int qtest_init(void)
+static inline int qtest_init(QEMUMachine *machine)
 {
     return 0;
 }
diff --git a/kvm-all.c b/kvm-all.c
index b87215c..3863abd 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -35,6 +35,8 @@
 #include "qemu/event_notifier.h"
 #include "trace.h"
 
+#include "hw/boards.h"
+
 /* This check must be after config-host.h is included */
 #ifdef CONFIG_EVENTFD
 #include <sys/eventfd.h>
@@ -1342,7 +1344,7 @@ static int kvm_max_vcpus(KVMState *s)
     return 4;
 }
 
-int kvm_init(void)
+int kvm_init(QEMUMachine *machine)
 {
     static const char upgrade_note[] =
         "Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n"
@@ -1350,7 +1352,7 @@ int kvm_init(void)
     KVMState *s;
     const KVMCapabilityInfo *missing_cap;
     int ret;
-    int i;
+    int i, kvm_type = 0;
     int max_vcpus;
 
     s = g_malloc0(sizeof(KVMState));
@@ -1407,7 +1409,11 @@ int kvm_init(void)
         goto err;
     }
 
-    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
+    if (machine->get_vm_type) {
+        kvm_type = machine->get_vm_type(qemu_opt_get(qemu_get_machine_opts(),
+                                                     "kvm_type"));
+    }
+    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, kvm_type);
     if (s->vmfd < 0) {
 #ifdef TARGET_S390X
         fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
diff --git a/kvm-stub.c b/kvm-stub.c
index 548f471..ccb7b8c 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -19,6 +19,8 @@
 #include "hw/pci/msi.h"
 #endif
 
+#include "hw/boards.h"
+
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupts_allowed;
@@ -33,7 +35,7 @@ int kvm_init_vcpu(CPUState *cpu)
     return -ENOSYS;
 }
 
-int kvm_init(void)
+int kvm_init(QEMUMachine *machine)
 {
     return -ENOSYS;
 }
diff --git a/qtest.c b/qtest.c
index 584c707..ef3c473 100644
--- a/qtest.c
+++ b/qtest.c
@@ -502,7 +502,7 @@ static void qtest_event(void *opaque, int event)
     }
 }
 
-int qtest_init(void)
+int qtest_init(QEMUMachine *machine)
 {
     CharDriverState *chr;
 
diff --git a/vl.c b/vl.c
index 4e709d5..7ecc581 100644
--- a/vl.c
+++ b/vl.c
@@ -427,7 +427,12 @@ static QemuOptsList qemu_machine_opts = {
             .name = "usb",
             .type = QEMU_OPT_BOOL,
             .help = "Set on/off to enable/disable usb",
+        },{
+            .name = "kvm_type",
+            .type = QEMU_OPT_STRING,
+            .help = "Set to kvm type to be used in create vm ioctl",
         },
+
         { /* End of list */ }
     },
 };
@@ -2608,7 +2613,7 @@ static QEMUMachine *machine_parse(const char *name)
     exit(!name || !is_help_option(name));
 }
 
-static int tcg_init(void)
+static int tcg_init(QEMUMachine *machine)
 {
     tcg_exec_init(tcg_tb_size * 1024 * 1024);
     return 0;
@@ -2618,7 +2623,7 @@ static struct {
     const char *opt_name;
     const char *name;
     int (*available)(void);
-    int (*init)(void);
+    int (*init)(QEMUMachine *);
     bool *allowed;
 } accel_list[] = {
     { "tcg", "tcg", tcg_available, tcg_init, &tcg_allowed },
@@ -2627,7 +2632,7 @@ static struct {
     { "qtest", "QTest", qtest_available, qtest_init, &qtest_allowed },
 };
 
-static int configure_accelerator(void)
+static int configure_accelerator(QEMUMachine *machine)
 {
     const char *p;
     char buf[10];
@@ -2654,7 +2659,7 @@ static int configure_accelerator(void)
                     continue;
                 }
                 *(accel_list[i].allowed) = true;
-                ret = accel_list[i].init();
+                ret = accel_list[i].init(machine);
                 if (ret < 0) {
                     init_failed = true;
                     fprintf(stderr, "failed to initialize %s: %s\n",
@@ -4037,10 +4042,10 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
-    configure_accelerator();
+    configure_accelerator(machine);
 
     if (!qtest_enabled() && qtest_chrdev) {
-        qtest_init();
+        qtest_init(machine);
     }
 
     machine_opts = qemu_get_machine_opts();
diff --git a/xen-all.c b/xen-all.c
index 839f14f..ac3654b 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -1000,7 +1000,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
     xs_daemon_close(state->xenstore);
 }
 
-int xen_init(void)
+int xen_init(QEMUMachine *machine)
 {
     xen_xc = xen_xc_interface_open(0, 0, 0);
     if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
diff --git a/xen-stub.c b/xen-stub.c
index ad189a6..59927cb 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -47,7 +47,7 @@ qemu_irq *xen_interrupt_controller_init(void)
     return NULL;
 }
 
-int xen_init(void)
+int xen_init(QEMUMachine *machine)
 {
     return -ENOSYS;
 }

>
>>
>>
>>
>>> Also, users don't want to say type=0. They want to say type=PR or
>>> type=HV or type=HV,PR. In fact, can't you make this a property of
>>> -accel? Then it's truly accel specific and everything should be well.
>> If we are doing this as machine property, we can't specify string,
>> because "HV"/"PR" are all powerpc dependent, so parsing that is not
>> possible in kvm_init in qemu. But, yes ideally it would be nice to be
>
> Well, we could do the "name to integer" conversion in an arch specific 
> function, no?
>
>> able to speicy the type using string. I thought accel is a machine
>> property, hence was not sure whether I can have additional properties
>> against that. I was using it as below.
>>
>>   -machine pseries,accel=kvm,kvm_type=1

Can we really specific -accel ? I check and I am finding that as machine
property. 

-aneesh

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Alexander Graf]

On 10/01/2013 01:26 PM, Aneesh Kumar K.V wrote:
> Alexander Graf<agraf@suse.de>  writes:
>
>> On 09/30/2013 03:09 PM, Aneesh Kumar K.V wrote:
>>> Alexander Graf<agraf@suse.de>   writes:
>>>
>>>> On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:
>>>>
>>>>> "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>   writes:
>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> This patch series support enabling HV and PR KVM together in the same kernel. We
>>>>>> extend machine property with new property "kvm_type". A value of 1 will force HV
>>>>>> KVM and 2 PR KVM. The default value is 0 which will select the fastest KVM mode.
>>>>>> ie, HV if that is supported otherwise PR.
>>>>>>
>>>>>> With Qemu command line having
>>>>>>
>>>>>> -machine pseries,accel=kvm,kvm_type=1
>>>>>>
>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>> failed to initialize KVM: Invalid argument
>>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>> failed to initialize KVM: Invalid argument
>>>>>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>
>>>>>> now with
>>>>>>
>>>>>> -machine pseries,accel=kvm,kvm_type=2
>>>>>>
>>>>>> [root@llmp24l02 qemu]# rmmod kvm-pr
>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>> failed to initialize KVM: Invalid argument
>>>>>> [root@llmp24l02 qemu]#
>>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>
>>>>>> if don't specify kvm_type machine property, it will take a default value 0,
>>>>>> which means fastest supported.
>>>>> Related qemu patch
>>>>>
>>>>> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
>>>>> Author: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>>>> Date:   Mon Sep 23 12:28:37 2013 +0530
>>>>>
>>>>>      kvm: Add a new machine property kvm_type
>>>>>
>>>>>      Targets like ppc64 support different type of KVM, one which use
>>>>>      hypervisor mode and the other which doesn't. Add a new machine
>>>>>      property kvm_type that helps in selecting the respective ones
>>>>>
>>>>>      Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>>> This really is too early, as we can't possibly run in HV mode for
>>>> non-pseries machines, so the interpretation (or at least sanity
>>>> checking) of what values are reasonable should occur in the
>>>> machine. That's why it's a variable in the "machine opts".
>>> With the current code CREATE_VM will fail, because we won't have
>>> kvm-hv.ko loaded and trying to create a vm with type 1 will fail.
>>> Now the challenge related to moving that to machine_init or later is, we
>>> depend on HV or PR callback early in CREATE_VM. With the changes we have
>>>
>>> int kvmppc_core_init_vm(struct kvm *kvm)
>>> {
>>>
>>> #ifdef CONFIG_PPC64
>>> 	INIT_LIST_HEAD(&kvm->arch.spapr_tce_tables);
>>> 	INIT_LIST_HEAD(&kvm->arch.rtas_tokens);
>>> #endif
>>>
>>> 	return kvm->arch.kvm_ops->init_vm(kvm);
>>> }
>>>
>>> Also the mmu notifier callback do end up calling kvm_unmap_hva etc which
>>> are all HV/PR dependent.
>> Yes, so we should verify in the machine models that we're runnable with
>> the currently selected type at least, to give the user a sensible error
>> message.
> Something like the below

I like that one a lot. Andreas, Paolo, what do you think?


Alex

>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 004184d..7d59ac1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1337,6 +1337,21 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>       assert(spapr->fdt_skel != NULL);
>   }
>
> +static int spapr_get_vm_type(const char *vm_type)
> +{
> +    if (!vm_type)
> +        return 0;
> +
> +    if (!strcmp(vm_type, "HV"))
> +        return 1;
> +
> +    if (!strcmp(vm_type, "PR"))
> +        return 2;
> +
> +    hw_error("qemu: unknown kvm_type specified '%s'", vm_type);
> +    exit(1);
> +}
> +
>   static QEMUMachine spapr_machine = {
>       .name = "pseries",
>       .desc = "pSeries Logical Partition (PAPR compliant)",
> @@ -1347,6 +1362,7 @@ static QEMUMachine spapr_machine = {
>       .max_cpus = MAX_CPUS,
>       .no_parallel = 1,
>       .default_boot_order = NULL,
> +    .get_vm_type = spapr_get_vm_type,
>   };
>
>   static void spapr_machine_init(void)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5a7ae9f..2130488 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -21,6 +21,8 @@ typedef void QEMUMachineResetFunc(void);
>
>   typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
>
> +typedef int QEMUMachineGetVmTypeFunc(const char *arg);
> +
>   typedef struct QEMUMachine {
>       const char *name;
>       const char *alias;
> @@ -28,6 +30,7 @@ typedef struct QEMUMachine {
>       QEMUMachineInitFunc *init;
>       QEMUMachineResetFunc *reset;
>       QEMUMachineHotAddCPUFunc *hot_add_cpu;
> +    QEMUMachineGetVmTypeFunc *get_vm_type;
>       BlockInterfaceType block_default_type;
>       int max_cpus;
>       unsigned int no_serial:1,
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index e1f88bf..acc3d74 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -36,7 +36,8 @@ void xen_cmos_set_s3_resume(void *opaque, int irq, int level);
>
>   qemu_irq *xen_interrupt_controller_init(void);
>
> -int xen_init(void);
> +typedef struct QEMUMachine QEMUMachine;
> +int xen_init(QEMUMachine *machine);
>   int xen_hvm_init(MemoryRegion **ram_memory);
>   void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 9bbe3db..f25caec 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -142,8 +142,8 @@ typedef struct KVMState KVMState;
>   extern KVMState *kvm_state;
>
>   /* external API */
> -
> -int kvm_init(void);
> +typedef struct QEMUMachine QEMUMachine;
> +int kvm_init(QEMUMachine *machine);
>
>   int kvm_has_sync_mmu(void);
>   int kvm_has_vcpu_events(void);
> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
> index 9a0c6b3..d71343d 100644
> --- a/include/sysemu/qtest.h
> +++ b/include/sysemu/qtest.h
> @@ -31,7 +31,8 @@ static inline int qtest_available(void)
>       return 1;
>   }
>
> -int qtest_init(void);
> +typedef struct QEMUMachine QEMUMachine;
> +int qtest_init(QEMUMachine *machine);
>   #else
>   static inline bool qtest_enabled(void)
>   {
> @@ -43,7 +44,7 @@ static inline int qtest_available(void)
>       return 0;
>   }
>
> -static inline int qtest_init(void)
> +static inline int qtest_init(QEMUMachine *machine)
>   {
>       return 0;
>   }
> diff --git a/kvm-all.c b/kvm-all.c
> index b87215c..3863abd 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -35,6 +35,8 @@
>   #include "qemu/event_notifier.h"
>   #include "trace.h"
>
> +#include "hw/boards.h"
> +
>   /* This check must be after config-host.h is included */
>   #ifdef CONFIG_EVENTFD
>   #include<sys/eventfd.h>
> @@ -1342,7 +1344,7 @@ static int kvm_max_vcpus(KVMState *s)
>       return 4;
>   }
>
> -int kvm_init(void)
> +int kvm_init(QEMUMachine *machine)
>   {
>       static const char upgrade_note[] =
>           "Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n"
> @@ -1350,7 +1352,7 @@ int kvm_init(void)
>       KVMState *s;
>       const KVMCapabilityInfo *missing_cap;
>       int ret;
> -    int i;
> +    int i, kvm_type = 0;
>       int max_vcpus;
>
>       s = g_malloc0(sizeof(KVMState));
> @@ -1407,7 +1409,11 @@ int kvm_init(void)
>           goto err;
>       }
>
> -    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> +    if (machine->get_vm_type) {
> +        kvm_type = machine->get_vm_type(qemu_opt_get(qemu_get_machine_opts(),
> +                                                     "kvm_type"));
> +    }
> +    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, kvm_type);
>       if (s->vmfd<  0) {
>   #ifdef TARGET_S390X
>           fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
> diff --git a/kvm-stub.c b/kvm-stub.c
> index 548f471..ccb7b8c 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -19,6 +19,8 @@
>   #include "hw/pci/msi.h"
>   #endif
>
> +#include "hw/boards.h"
> +
>   KVMState *kvm_state;
>   bool kvm_kernel_irqchip;
>   bool kvm_async_interrupts_allowed;
> @@ -33,7 +35,7 @@ int kvm_init_vcpu(CPUState *cpu)
>       return -ENOSYS;
>   }
>
> -int kvm_init(void)
> +int kvm_init(QEMUMachine *machine)
>   {
>       return -ENOSYS;
>   }
> diff --git a/qtest.c b/qtest.c
> index 584c707..ef3c473 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -502,7 +502,7 @@ static void qtest_event(void *opaque, int event)
>       }
>   }
>
> -int qtest_init(void)
> +int qtest_init(QEMUMachine *machine)
>   {
>       CharDriverState *chr;
>
> diff --git a/vl.c b/vl.c
> index 4e709d5..7ecc581 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -427,7 +427,12 @@ static QemuOptsList qemu_machine_opts = {
>               .name = "usb",
>               .type = QEMU_OPT_BOOL,
>               .help = "Set on/off to enable/disable usb",
> +        },{
> +            .name = "kvm_type",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Set to kvm type to be used in create vm ioctl",
>           },
> +
>           { /* End of list */ }
>       },
>   };
> @@ -2608,7 +2613,7 @@ static QEMUMachine *machine_parse(const char *name)
>       exit(!name || !is_help_option(name));
>   }
>
> -static int tcg_init(void)
> +static int tcg_init(QEMUMachine *machine)
>   {
>       tcg_exec_init(tcg_tb_size * 1024 * 1024);
>       return 0;
> @@ -2618,7 +2623,7 @@ static struct {
>       const char *opt_name;
>       const char *name;
>       int (*available)(void);
> -    int (*init)(void);
> +    int (*init)(QEMUMachine *);
>       bool *allowed;
>   } accel_list[] = {
>       { "tcg", "tcg", tcg_available, tcg_init,&tcg_allowed },
> @@ -2627,7 +2632,7 @@ static struct {
>       { "qtest", "QTest", qtest_available, qtest_init,&qtest_allowed },
>   };
>
> -static int configure_accelerator(void)
> +static int configure_accelerator(QEMUMachine *machine)
>   {
>       const char *p;
>       char buf[10];
> @@ -2654,7 +2659,7 @@ static int configure_accelerator(void)
>                       continue;
>                   }
>                   *(accel_list[i].allowed) = true;
> -                ret = accel_list[i].init();
> +                ret = accel_list[i].init(machine);
>                   if (ret<  0) {
>                       init_failed = true;
>                       fprintf(stderr, "failed to initialize %s: %s\n",
> @@ -4037,10 +4042,10 @@ int main(int argc, char **argv, char **envp)
>           exit(0);
>       }
>
> -    configure_accelerator();
> +    configure_accelerator(machine);
>
>       if (!qtest_enabled()&&  qtest_chrdev) {
> -        qtest_init();
> +        qtest_init(machine);
>       }
>
>       machine_opts = qemu_get_machine_opts();
> diff --git a/xen-all.c b/xen-all.c
> index 839f14f..ac3654b 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -1000,7 +1000,7 @@ static void xen_exit_notifier(Notifier *n, void *data)
>       xs_daemon_close(state->xenstore);
>   }
>
> -int xen_init(void)
> +int xen_init(QEMUMachine *machine)
>   {
>       xen_xc = xen_xc_interface_open(0, 0, 0);
>       if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
> diff --git a/xen-stub.c b/xen-stub.c
> index ad189a6..59927cb 100644
> --- a/xen-stub.c
> +++ b/xen-stub.c
> @@ -47,7 +47,7 @@ qemu_irq *xen_interrupt_controller_init(void)
>       return NULL;
>   }
>
> -int xen_init(void)
> +int xen_init(QEMUMachine *machine)
>   {
>       return -ENOSYS;
>   }
>
>>>
>>>
>>>> Also, users don't want to say type=0. They want to say type=PR or
>>>> type=HV or type=HV,PR. In fact, can't you make this a property of
>>>> -accel? Then it's truly accel specific and everything should be well.
>>> If we are doing this as machine property, we can't specify string,
>>> because "HV"/"PR" are all powerpc dependent, so parsing that is not
>>> possible in kvm_init in qemu. But, yes ideally it would be nice to be
>> Well, we could do the "name to integer" conversion in an arch specific
>> function, no?
>>
>>> able to speicy the type using string. I thought accel is a machine
>>> property, hence was not sure whether I can have additional properties
>>> against that. I was using it as below.
>>>
>>>    -machine pseries,accel=kvm,kvm_type=1
> Can we really specific -accel ? I check and I am finding that as machine
> property.
>
> -aneesh
>

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Paolo Bonzini]

Il 01/10/2013 13:36, Alexander Graf ha scritto:
>>>>
>>> Yes, so we should verify in the machine models that we're runnable with
>>> the currently selected type at least, to give the user a sensible error
>>> message.
>> Something like the below
> 
> I like that one a lot. Andreas, Paolo, what do you think?

Yes, it's fine.

Paolo

Re: [RFC PATCH 00/11 Allow PR and HV KVM to coexist in one kernel

[Alexander Graf]

On 10/01/2013 01:36 PM, Alexander Graf wrote:
> On 10/01/2013 01:26 PM, Aneesh Kumar K.V wrote:
>> Alexander Graf<agraf@suse.de>  writes:
>>
>>> On 09/30/2013 03:09 PM, Aneesh Kumar K.V wrote:
>>>> Alexander Graf<agraf@suse.de>   writes:
>>>>
>>>>> On 27.09.2013, at 12:52, Aneesh Kumar K.V wrote:
>>>>>
>>>>>> "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>   writes:
>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>> This patch series support enabling HV and PR KVM together in the 
>>>>>>> same kernel. We
>>>>>>> extend machine property with new property "kvm_type". A value of 
>>>>>>> 1 will force HV
>>>>>>> KVM and 2 PR KVM. The default value is 0 which will select the 
>>>>>>> fastest KVM mode.
>>>>>>> ie, HV if that is supported otherwise PR.
>>>>>>>
>>>>>>> With Qemu command line having
>>>>>>>
>>>>>>> -machine pseries,accel=kvm,kvm_type=1
>>>>>>>
>>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>> failed to initialize KVM: Invalid argument
>>>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>> failed to initialize KVM: Invalid argument
>>>>>>> [root@llmp24l02 qemu]# modprobe  kvm-hv
>>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>>
>>>>>>> now with
>>>>>>>
>>>>>>> -machine pseries,accel=kvm,kvm_type=2
>>>>>>>
>>>>>>> [root@llmp24l02 qemu]# rmmod kvm-pr
>>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>> failed to initialize KVM: Invalid argument
>>>>>>> [root@llmp24l02 qemu]#
>>>>>>> [root@llmp24l02 qemu]# modprobe kvm-pr
>>>>>>> [root@llmp24l02 qemu]# bash ../qemu
>>>>>>>
>>>>>>> if don't specify kvm_type machine property, it will take a 
>>>>>>> default value 0,
>>>>>>> which means fastest supported.
>>>>>> Related qemu patch
>>>>>>
>>>>>> commit 8d139053177d48a70cb710b211ea4c2843eccdfb
>>>>>> Author: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>>>>>> Date:   Mon Sep 23 12:28:37 2013 +0530
>>>>>>
>>>>>>      kvm: Add a new machine property kvm_type
>>>>>>
>>>>>>      Targets like ppc64 support different type of KVM, one which use
>>>>>>      hypervisor mode and the other which doesn't. Add a new machine
>>>>>>      property kvm_type that helps in selecting the respective ones
>>>>>>
>>>>>>      Signed-off-by: Aneesh Kumar 
>>>>>> K.V<aneesh.kumar@linux.vnet.ibm.com>
>>>>> This really is too early, as we can't possibly run in HV mode for
>>>>> non-pseries machines, so the interpretation (or at least sanity
>>>>> checking) of what values are reasonable should occur in the
>>>>> machine. That's why it's a variable in the "machine opts".
>>>> With the current code CREATE_VM will fail, because we won't have
>>>> kvm-hv.ko loaded and trying to create a vm with type 1 will fail.
>>>> Now the challenge related to moving that to machine_init or later 
>>>> is, we
>>>> depend on HV or PR callback early in CREATE_VM. With the changes we 
>>>> have
>>>>
>>>> int kvmppc_core_init_vm(struct kvm *kvm)
>>>> {
>>>>
>>>> #ifdef CONFIG_PPC64
>>>>     INIT_LIST_HEAD(&kvm->arch.spapr_tce_tables);
>>>>     INIT_LIST_HEAD(&kvm->arch.rtas_tokens);
>>>> #endif
>>>>
>>>>     return kvm->arch.kvm_ops->init_vm(kvm);
>>>> }
>>>>
>>>> Also the mmu notifier callback do end up calling kvm_unmap_hva etc 
>>>> which
>>>> are all HV/PR dependent.
>>> Yes, so we should verify in the machine models that we're runnable with
>>> the currently selected type at least, to give the user a sensible error
>>> message.
>> Something like the below
>
> I like that one a lot. Andreas, Paolo, what do you think?

To clarify this a bit more, this patch is missing a few critical pieces:

   1) The default implementation for the kvm_type callback needs to 
error out when kvm_type is set to anything
   2) All non-papr-non-e500-PPC machines should force "PR only" mode and 
bail out on anything else
   3) We're missing sensible error messages for the user still when the 
VM could not get created

but I like the overall concept :).


Alex