| Message ID | 1598256668-12131-1-git-send-email-chenhc@lemote.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS | expand |
Hi Huacai, On 8/24/20 10:11 AM, Huacai Chen wrote: > MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, > libvirt uses a null-machine to detect the kvm capability. In the MIPS > case, it will return "KVM not supported" on a VZ platform by default. > So, add the kvm_type() hook to the null-machine. > > This seems not a very good solution, but I cannot do it better now. > > Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > Signed-off-by: Huacai Chen <chenhc@lemote.com> > Co-developed-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > hw/core/meson.build | 2 +- > hw/core/null-machine.c | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/core/meson.build b/hw/core/meson.build > index fc91f98..b6591b9 100644 > --- a/hw/core/meson.build > +++ b/hw/core/meson.build > @@ -35,7 +35,6 @@ softmmu_ss.add(files( > 'machine-hmp-cmds.c', > 'machine.c', > 'nmi.c', > - 'null-machine.c', > 'qdev-fw.c', > 'qdev-properties-system.c', > 'sysbus.c', > @@ -45,5 +44,6 @@ softmmu_ss.add(files( > > specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files( > 'machine-qmp-cmds.c', > + 'null-machine.c', > 'numa.c', > )) > diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c > index 7e69352..4b4ab76 100644 > --- a/hw/core/null-machine.c > +++ b/hw/core/null-machine.c > @@ -17,6 +17,9 @@ > #include "sysemu/sysemu.h" > #include "exec/address-spaces.h" > #include "hw/core/cpu.h" > +#ifdef TARGET_MIPS > +#include "kvm_mips.h" > +#endif > > static void machine_none_init(MachineState *mch) > { > @@ -55,6 +58,9 @@ static void machine_none_machine_init(MachineClass *mc) > mc->no_floppy = 1; > mc->no_cdrom = 1; > mc->no_sdcard = 1; > +#ifdef TARGET_MIPS > + mc->kvm_type = mips_kvm_type; > +#endif I'm a bit confused here, you are taking the same path from your v4... https://www.mail-archive.com/qemu-devel@nongnu.org/msg712550.html Did you rebase the correct version? > } > > DEFINE_MACHINE("none", machine_none_machine_init) >
Hi, Philippe, On Mon, Sep 7, 2020 at 11:39 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > You forgot to Cc the maintainers, doing it for you: > > ./scripts/get_maintainer.pl -f hw/core/null-machine.c > Eduardo Habkost <ehabkost@redhat.com> (supporter:Machine core) > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:Machine core) Thank you very much! Huacai > > On 9/3/20 2:58 AM, Huacai Chen wrote: > > Hi, Philippe, > > > > On Wed, Sep 2, 2020 at 9:55 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> > >> Hi Huacai, > >> > >> On 8/24/20 10:11 AM, Huacai Chen wrote: > >>> MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, > >>> libvirt uses a null-machine to detect the kvm capability. In the MIPS > >>> case, it will return "KVM not supported" on a VZ platform by default. > >>> So, add the kvm_type() hook to the null-machine. > >>> > >>> This seems not a very good solution, but I cannot do it better now. > >>> > >>> Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > >>> Signed-off-by: Huacai Chen <chenhc@lemote.com> > >>> Co-developed-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > >>> --- > >>> hw/core/meson.build | 2 +- > >>> hw/core/null-machine.c | 6 ++++++ > >>> 2 files changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/core/meson.build b/hw/core/meson.build > >>> index fc91f98..b6591b9 100644 > >>> --- a/hw/core/meson.build > >>> +++ b/hw/core/meson.build > >>> @@ -35,7 +35,6 @@ softmmu_ss.add(files( > >>> 'machine-hmp-cmds.c', > >>> 'machine.c', > >>> 'nmi.c', > >>> - 'null-machine.c', > >>> 'qdev-fw.c', > >>> 'qdev-properties-system.c', > >>> 'sysbus.c', > >>> @@ -45,5 +44,6 @@ softmmu_ss.add(files( > >>> > >>> specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files( > >>> 'machine-qmp-cmds.c', > >>> + 'null-machine.c', > >>> 'numa.c', > >>> )) > >>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c > >>> index 7e69352..4b4ab76 100644 > >>> --- a/hw/core/null-machine.c > >>> +++ b/hw/core/null-machine.c > >>> @@ -17,6 +17,9 @@ > >>> #include "sysemu/sysemu.h" > >>> #include "exec/address-spaces.h" > >>> #include "hw/core/cpu.h" > >>> +#ifdef TARGET_MIPS > >>> +#include "kvm_mips.h" > >>> +#endif > >>> > >>> static void machine_none_init(MachineState *mch) > >>> { > >>> @@ -55,6 +58,9 @@ static void machine_none_machine_init(MachineClass *mc) > >>> mc->no_floppy = 1; > >>> mc->no_cdrom = 1; > >>> mc->no_sdcard = 1; > >>> +#ifdef TARGET_MIPS > >>> + mc->kvm_type = mips_kvm_type; > >>> +#endif > >> > >> I'm a bit confused here, you are taking the same path from your v4... > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg712550.html > >> > >> Did you rebase the correct version? > > The old patch has split to two parts, the first part is used by MIPS > > machine and already merged. This is the second part used by the > > null-machine (and libvirt use null-machine to detect kvm > > capabilities). > > > > Huacai > >> > >>> } > >>> > >>> DEFINE_MACHINE("none", machine_none_machine_init) > >>> > >
On Tue, Sep 08, 2020 at 07:25:47PM +0200, Thomas Huth wrote: > On 24/08/2020 10.11, Huacai Chen wrote: > > MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, > > libvirt uses a null-machine to detect the kvm capability. In the MIPS > > case, it will return "KVM not supported" on a VZ platform by default. > > So, add the kvm_type() hook to the null-machine. > > > > This seems not a very good solution, but I cannot do it better now. > > This is still ugly. Why do the other architectures do not have the > same problem? Let's see... in kvm-all.c, we have: > > int type = 0; > [...] > kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type"); > if (mc->kvm_type) { > type = mc->kvm_type(ms, kvm_type); > } else if (kvm_type) { > ret = -EINVAL; > fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type); > goto err; > } > > do { > ret = kvm_ioctl(s, KVM_CREATE_VM, type); > } while (ret == -EINTR); > > Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this > case (i.e. when libvirt probes with the "null"-machine). > > Now let's have a look at the kernel. The "type" parameter is passed > there to the architecture specific function kvm_arch_init_vm(). > For powerpc, this looks like: > > if (type == 0) { > if (kvmppc_hv_ops) > kvm_ops = kvmppc_hv_ops; > else > kvm_ops = kvmppc_pr_ops; > if (!kvm_ops) > goto err_out; > } else if (type == KVM_VM_PPC_HV) { > if (!kvmppc_hv_ops) > goto err_out; > kvm_ops = kvmppc_hv_ops; > } else if (type == KVM_VM_PPC_PR) { > if (!kvmppc_pr_ops) > goto err_out; > kvm_ops = kvmppc_pr_ops; > } else > goto err_out; > > That means for type == 0, it automatically detects the best > kvm-type. > > For mips, this function looks like this: > > switch (type) { > #ifdef CONFIG_KVM_MIPS_VZ > case KVM_VM_MIPS_VZ: > #else > case KVM_VM_MIPS_TE: > #endif > break; > default: > /* Unsupported KVM type */ > return -EINVAL; > }; > > That means, for type == 0, it returns -EINVAL here! > > Looking at the API docu in Documentation/virt/kvm/api.rst > the description of the type parameter is quite sparse, but it > says: > > "You probably want to use 0 as machine type." > > So I think this is a bug in the implementation of KVM in the > mips kernel code. The kvm_arch_init_vm() in the mips code should > do the same as on powerpc, and use the best available KVM type > there instead of returning EINVAL. Once that is fixed there, > you don't need this patch here for QEMU anymore. If there's a way to make it work with older kernels, I assume we would still want to do it. However, this kind of kvm-specific + arch-specific knowledge doesn't belong to machine core code. If we are going to add a #ifdef TARGET_MIPS to the code, we can simply do it inside kvm-all.c:kvm_init().
On 9/8/20 7:59 PM, Eduardo Habkost wrote: > On Tue, Sep 08, 2020 at 07:25:47PM +0200, Thomas Huth wrote: >> On 24/08/2020 10.11, Huacai Chen wrote: >>> MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, >>> libvirt uses a null-machine to detect the kvm capability. In the MIPS >>> case, it will return "KVM not supported" on a VZ platform by default. >>> So, add the kvm_type() hook to the null-machine. >>> >>> This seems not a very good solution, but I cannot do it better now. >> >> This is still ugly. Why do the other architectures do not have the >> same problem? Let's see... in kvm-all.c, we have: >> >> int type = 0; >> [...] >> kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type"); >> if (mc->kvm_type) { >> type = mc->kvm_type(ms, kvm_type); >> } else if (kvm_type) { >> ret = -EINVAL; >> fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type); >> goto err; >> } >> >> do { >> ret = kvm_ioctl(s, KVM_CREATE_VM, type); >> } while (ret == -EINTR); >> >> Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this >> case (i.e. when libvirt probes with the "null"-machine). >> >> Now let's have a look at the kernel. The "type" parameter is passed >> there to the architecture specific function kvm_arch_init_vm(). >> For powerpc, this looks like: >> >> if (type == 0) { >> if (kvmppc_hv_ops) >> kvm_ops = kvmppc_hv_ops; >> else >> kvm_ops = kvmppc_pr_ops; >> if (!kvm_ops) >> goto err_out; >> } else if (type == KVM_VM_PPC_HV) { >> if (!kvmppc_hv_ops) >> goto err_out; >> kvm_ops = kvmppc_hv_ops; >> } else if (type == KVM_VM_PPC_PR) { >> if (!kvmppc_pr_ops) >> goto err_out; >> kvm_ops = kvmppc_pr_ops; >> } else >> goto err_out; >> >> That means for type == 0, it automatically detects the best >> kvm-type. >> >> For mips, this function looks like this: >> >> switch (type) { >> #ifdef CONFIG_KVM_MIPS_VZ >> case KVM_VM_MIPS_VZ: >> #else >> case KVM_VM_MIPS_TE: >> #endif >> break; >> default: >> /* Unsupported KVM type */ >> return -EINVAL; >> }; >> >> That means, for type == 0, it returns -EINVAL here! >> >> Looking at the API docu in Documentation/virt/kvm/api.rst >> the description of the type parameter is quite sparse, but it >> says: >> >> "You probably want to use 0 as machine type." >> >> So I think this is a bug in the implementation of KVM in the >> mips kernel code. The kvm_arch_init_vm() in the mips code should >> do the same as on powerpc, and use the best available KVM type >> there instead of returning EINVAL. Once that is fixed there, >> you don't need this patch here for QEMU anymore. > > If there's a way to make it work with older kernels, I assume we > would still want to do it. IIUC this is available since kernel v5.8-rc1 (merged in 52cd0d972fa6) less than 3 months ago. v5.9-rc4 just got released, not sure if this can be fixed for the v5.9 release. Is this small older kernels window important? Huacai, can you amend in the commit description since when the feature is available in the Linux kernel please? (just for informative purpose). > > However, this kind of kvm-specific + arch-specific knowledge > doesn't belong to machine core code. If we are going to add a > #ifdef TARGET_MIPS to the code, we can simply do it inside > kvm-all.c:kvm_init(). >
Hi, all, On Wed, Sep 9, 2020 at 1:25 AM Thomas Huth <thuth@redhat.com> wrote: > > On 24/08/2020 10.11, Huacai Chen wrote: > > MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, > > libvirt uses a null-machine to detect the kvm capability. In the MIPS > > case, it will return "KVM not supported" on a VZ platform by default. > > So, add the kvm_type() hook to the null-machine. > > > > This seems not a very good solution, but I cannot do it better now. > > This is still ugly. Why do the other architectures do not have the > same problem? Let's see... in kvm-all.c, we have: > > int type = 0; > [...] > kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type"); > if (mc->kvm_type) { > type = mc->kvm_type(ms, kvm_type); > } else if (kvm_type) { > ret = -EINVAL; > fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type); > goto err; > } > > do { > ret = kvm_ioctl(s, KVM_CREATE_VM, type); > } while (ret == -EINTR); > > Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this > case (i.e. when libvirt probes with the "null"-machine). > > Now let's have a look at the kernel. The "type" parameter is passed > there to the architecture specific function kvm_arch_init_vm(). > For powerpc, this looks like: > > if (type == 0) { > if (kvmppc_hv_ops) > kvm_ops = kvmppc_hv_ops; > else > kvm_ops = kvmppc_pr_ops; > if (!kvm_ops) > goto err_out; > } else if (type == KVM_VM_PPC_HV) { > if (!kvmppc_hv_ops) > goto err_out; > kvm_ops = kvmppc_hv_ops; > } else if (type == KVM_VM_PPC_PR) { > if (!kvmppc_pr_ops) > goto err_out; > kvm_ops = kvmppc_pr_ops; > } else > goto err_out; > > That means for type == 0, it automatically detects the best > kvm-type. > > For mips, this function looks like this: > > switch (type) { > #ifdef CONFIG_KVM_MIPS_VZ > case KVM_VM_MIPS_VZ: > #else > case KVM_VM_MIPS_TE: > #endif > break; > default: > /* Unsupported KVM type */ > return -EINVAL; > }; > > That means, for type == 0, it returns -EINVAL here! > > Looking at the API docu in Documentation/virt/kvm/api.rst > the description of the type parameter is quite sparse, but it > says: > > "You probably want to use 0 as machine type." > > So I think this is a bug in the implementation of KVM in the > mips kernel code. The kvm_arch_init_vm() in the mips code should > do the same as on powerpc, and use the best available KVM type > there instead of returning EINVAL. Once that is fixed there, > you don't need this patch here for QEMU anymore. Yes, PPC use a good method, because it can use 0 as "automatic" #define KVM_VM_PPC_HV 1 #define KVM_VM_PPC_PR 2 Unfortunately, MIPS cannot do like this because it define 0 as "TE": #define KVM_VM_MIPS_TE 0 #define KVM_VM_MIPS_VZ 1 So, it cannot be solved in kernel side, unless changing the definition of TE/VZ, but I think changing their definition is also unacceptable. Huacai > > HTH, > Thomas > > > > Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > > Signed-off-by: Huacai Chen <chenhc@lemote.com> > > Co-developed-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > > --- > > hw/core/meson.build | 2 +- > > hw/core/null-machine.c | 6 ++++++ > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/meson.build b/hw/core/meson.build > > index fc91f98..b6591b9 100644 > > --- a/hw/core/meson.build > > +++ b/hw/core/meson.build > > @@ -35,7 +35,6 @@ softmmu_ss.add(files( > > 'machine-hmp-cmds.c', > > 'machine.c', > > 'nmi.c', > > - 'null-machine.c', > > 'qdev-fw.c', > > 'qdev-properties-system.c', > > 'sysbus.c', > > @@ -45,5 +44,6 @@ softmmu_ss.add(files( > > > > specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files( > > 'machine-qmp-cmds.c', > > + 'null-machine.c', > > 'numa.c', > > )) > > diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c > > index 7e69352..4b4ab76 100644 > > --- a/hw/core/null-machine.c > > +++ b/hw/core/null-machine.c > > @@ -17,6 +17,9 @@ > > #include "sysemu/sysemu.h" > > #include "exec/address-spaces.h" > > #include "hw/core/cpu.h" > > +#ifdef TARGET_MIPS > > +#include "kvm_mips.h" > > +#endif > > > > static void machine_none_init(MachineState *mch) > > { > > @@ -55,6 +58,9 @@ static void machine_none_machine_init(MachineClass *mc) > > mc->no_floppy = 1; > > mc->no_cdrom = 1; > > mc->no_sdcard = 1; > > +#ifdef TARGET_MIPS > > + mc->kvm_type = mips_kvm_type; > > +#endif > > } > > > > DEFINE_MACHINE("none", machine_none_machine_init) > > >
Hi, Thomas, On Wed, Sep 9, 2020 at 3:20 PM Thomas Huth <thuth@redhat.com> wrote: > > On 09/09/2020 04.57, chen huacai wrote: > > Hi, all, > > > > On Wed, Sep 9, 2020 at 1:25 AM Thomas Huth <thuth@redhat.com> wrote: > >> > >> On 24/08/2020 10.11, Huacai Chen wrote: > >>> MIPS has two types of KVM: TE & VZ, and TE is the default type. Now, > >>> libvirt uses a null-machine to detect the kvm capability. In the MIPS > >>> case, it will return "KVM not supported" on a VZ platform by default. > >>> So, add the kvm_type() hook to the null-machine. > >>> > >>> This seems not a very good solution, but I cannot do it better now. > >> > >> This is still ugly. Why do the other architectures do not have the > >> same problem? Let's see... in kvm-all.c, we have: > >> > >> int type = 0; > >> [...] > >> kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type"); > >> if (mc->kvm_type) { > >> type = mc->kvm_type(ms, kvm_type); > >> } else if (kvm_type) { > >> ret = -EINVAL; > >> fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type); > >> goto err; > >> } > >> > >> do { > >> ret = kvm_ioctl(s, KVM_CREATE_VM, type); > >> } while (ret == -EINTR); > >> > >> Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this > >> case (i.e. when libvirt probes with the "null"-machine). > >> > >> Now let's have a look at the kernel. The "type" parameter is passed > >> there to the architecture specific function kvm_arch_init_vm(). > >> For powerpc, this looks like: > >> > >> if (type == 0) { > >> if (kvmppc_hv_ops) > >> kvm_ops = kvmppc_hv_ops; > >> else > >> kvm_ops = kvmppc_pr_ops; > >> if (!kvm_ops) > >> goto err_out; > >> } else if (type == KVM_VM_PPC_HV) { > >> if (!kvmppc_hv_ops) > >> goto err_out; > >> kvm_ops = kvmppc_hv_ops; > >> } else if (type == KVM_VM_PPC_PR) { > >> if (!kvmppc_pr_ops) > >> goto err_out; > >> kvm_ops = kvmppc_pr_ops; > >> } else > >> goto err_out; > >> > >> That means for type == 0, it automatically detects the best > >> kvm-type. > >> > >> For mips, this function looks like this: > >> > >> switch (type) { > >> #ifdef CONFIG_KVM_MIPS_VZ > >> case KVM_VM_MIPS_VZ: > >> #else > >> case KVM_VM_MIPS_TE: > >> #endif > >> break; > >> default: > >> /* Unsupported KVM type */ > >> return -EINVAL; > >> }; > >> > >> That means, for type == 0, it returns -EINVAL here! > >> > >> Looking at the API docu in Documentation/virt/kvm/api.rst > >> the description of the type parameter is quite sparse, but it > >> says: > >> > >> "You probably want to use 0 as machine type." > >> > >> So I think this is a bug in the implementation of KVM in the > >> mips kernel code. The kvm_arch_init_vm() in the mips code should > >> do the same as on powerpc, and use the best available KVM type > >> there instead of returning EINVAL. Once that is fixed there, > >> you don't need this patch here for QEMU anymore. > > Yes, PPC use a good method, because it can use 0 as "automatic" > > #define KVM_VM_PPC_HV 1 > > #define KVM_VM_PPC_PR 2 > > > > Unfortunately, MIPS cannot do like this because it define 0 as "TE": > > #define KVM_VM_MIPS_TE 0 > > #define KVM_VM_MIPS_VZ 1 > > > > So, it cannot be solved in kernel side, unless changing the definition > > of TE/VZ, but I think changing their definition is also unacceptable. > > Ouch, ok, now I understood the problem. That sounds like a really bad > decision on the kernel side. > > But I think you could at least try to get it fixed on the kernel side: > Propose a patch to rename KVM_VM_MIPS_TE to KVM_VM_MIPS_DEFAULT and use > 2 as new value for TE. The code that handles the default 0 case should > then prefer TE over VZ, so that old userspace applications that provide > 0 will continue to work as they did before, so I hope that the change is > acceptable by the kernel folks. You should add a remark to the patch > description that 0 is the established default value for probing in the > QEMU/libvirt stack and that your patch is required on the kernel side to > avoid ugly hacks in QEMU userspace code. > > If they still reject your patch, I guess we have to bite the bullet and > add your patch here... OK, let me try. Huacai > > Thanks, > Thomas >
diff --git a/hw/core/meson.build b/hw/core/meson.build index fc91f98..b6591b9 100644 --- a/hw/core/meson.build +++ b/hw/core/meson.build @@ -35,7 +35,6 @@ softmmu_ss.add(files( 'machine-hmp-cmds.c', 'machine.c', 'nmi.c', - 'null-machine.c', 'qdev-fw.c', 'qdev-properties-system.c', 'sysbus.c', @@ -45,5 +44,6 @@ softmmu_ss.add(files( specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files( 'machine-qmp-cmds.c', + 'null-machine.c', 'numa.c', )) diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c index 7e69352..4b4ab76 100644 --- a/hw/core/null-machine.c +++ b/hw/core/null-machine.c @@ -17,6 +17,9 @@ #include "sysemu/sysemu.h" #include "exec/address-spaces.h" #include "hw/core/cpu.h" +#ifdef TARGET_MIPS +#include "kvm_mips.h" +#endif static void machine_none_init(MachineState *mch) { @@ -55,6 +58,9 @@ static void machine_none_machine_init(MachineClass *mc) mc->no_floppy = 1; mc->no_cdrom = 1; mc->no_sdcard = 1; +#ifdef TARGET_MIPS + mc->kvm_type = mips_kvm_type; +#endif } DEFINE_MACHINE("none", machine_none_machine_init)