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

login
register
mail settings
Submitter Aneesh Kumar K.V
Date Sept. 27, 2013, 10:52 a.m.
Message ID <878uyibkq2.fsf@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/278542/
State Not Applicable
Headers show

Comments

Aneesh Kumar K.V - Sept. 27, 2013, 10:52 a.m.
"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>
Alexander Graf - Sept. 30, 2013, 10:16 a.m.
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
Aneesh Kumar K.V - Sept. 30, 2013, 1:09 p.m.
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
Alexander Graf - Sept. 30, 2013, 2:54 p.m.
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

Patch

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 */ }
     },
 };