| Message ID | 878uyibkq2.fsf@linux.vnet.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 */ } }, };