* [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS @ 2020-08-24 8:11 Huacai Chen 2020-09-02 13:55 ` Philippe Mathieu-Daudé 2020-09-08 17:25 ` Thomas Huth 0 siblings, 2 replies; 11+ messages in thread From: Huacai Chen @ 2020-08-24 8:11 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Aleksandar Markovic Cc: Huacai Chen, Huacai Chen, Aleksandar Rikalo, qemu-devel, Aurelien Jarno 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 } DEFINE_MACHINE("none", machine_none_machine_init) -- 2.7.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-08-24 8:11 [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS Huacai Chen @ 2020-09-02 13:55 ` Philippe Mathieu-Daudé 2020-09-03 0:58 ` Huacai Chen 2020-09-08 17:25 ` Thomas Huth 1 sibling, 1 reply; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-02 13:55 UTC (permalink / raw) To: Huacai Chen, Aleksandar Markovic Cc: Aleksandar Rikalo, Eduardo Habkost, Huacai Chen, qemu-devel, Thomas Huth, Huacai Chen, Aurelien Jarno 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) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-02 13:55 ` Philippe Mathieu-Daudé @ 2020-09-03 0:58 ` Huacai Chen 2020-09-07 3:39 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 11+ messages in thread From: Huacai Chen @ 2020-09-03 0:58 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Huacai Chen, Aleksandar Rikalo, Eduardo Habkost, QEMU Developers, Aleksandar Markovic, Thomas Huth, Aurelien Jarno 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) > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-03 0:58 ` Huacai Chen @ 2020-09-07 3:39 ` Philippe Mathieu-Daudé 2020-09-08 0:41 ` Huacai Chen 0 siblings, 1 reply; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-07 3:39 UTC (permalink / raw) To: Huacai Chen Cc: Huacai Chen, Thomas Huth, Eduardo Habkost, Aleksandar Rikalo, QEMU Developers, Aleksandar Markovic, Aurelien Jarno 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) 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) >>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-07 3:39 ` Philippe Mathieu-Daudé @ 2020-09-08 0:41 ` Huacai Chen 0 siblings, 0 replies; 11+ messages in thread From: Huacai Chen @ 2020-09-08 0:41 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Huacai Chen, Thomas Huth, Eduardo Habkost, Aleksandar Rikalo, QEMU Developers, Aleksandar Markovic, Aurelien Jarno 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) > >>> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-08-24 8:11 [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS Huacai Chen 2020-09-02 13:55 ` Philippe Mathieu-Daudé @ 2020-09-08 17:25 ` Thomas Huth 2020-09-08 17:59 ` Eduardo Habkost 2020-09-09 2:57 ` chen huacai 1 sibling, 2 replies; 11+ messages in thread From: Thomas Huth @ 2020-09-08 17:25 UTC (permalink / raw) To: Huacai Chen, Philippe Mathieu-Daudé, Aleksandar Markovic Cc: Laurent Vivier, Peter Maydell, Aleksandar Rikalo, Huacai Chen, qemu-devel, Paolo Bonzini, David Gibson, Huacai Chen, Aurelien Jarno 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. 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) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-08 17:25 ` Thomas Huth @ 2020-09-08 17:59 ` Eduardo Habkost 2020-09-08 18:12 ` Philippe Mathieu-Daudé 2020-09-09 2:57 ` chen huacai 1 sibling, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2020-09-08 17:59 UTC (permalink / raw) To: Thomas Huth Cc: Laurent Vivier, Huacai Chen, Aleksandar Rikalo, Peter Maydell, Huacai Chen, Philippe Mathieu-Daudé, qemu-devel, Aleksandar Markovic, David Gibson, Huacai Chen, Paolo Bonzini, Aurelien Jarno 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(). -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-08 17:59 ` Eduardo Habkost @ 2020-09-08 18:12 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-08 18:12 UTC (permalink / raw) To: Eduardo Habkost, Thomas Huth, Paolo Bonzini Cc: Laurent Vivier, Huacai Chen, Aleksandar Rikalo, Peter Maydell, Huacai Chen, qemu-devel, Aleksandar Markovic, Huacai Chen, David Gibson, Aurelien Jarno 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(). > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-08 17:25 ` Thomas Huth 2020-09-08 17:59 ` Eduardo Habkost @ 2020-09-09 2:57 ` chen huacai 2020-09-09 7:20 ` Thomas Huth 1 sibling, 1 reply; 11+ messages in thread From: chen huacai @ 2020-09-09 2:57 UTC (permalink / raw) To: Thomas Huth Cc: Laurent Vivier, Peter Maydell, Aleksandar Rikalo, Huacai Chen, Philippe Mathieu-Daudé, qemu-level, Aleksandar Markovic, Paolo Bonzini, David Gibson, Huacai Chen, Aurelien Jarno 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) > > > -- Huacai Chen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-09 2:57 ` chen huacai @ 2020-09-09 7:20 ` Thomas Huth 2020-09-10 1:31 ` Huacai Chen 0 siblings, 1 reply; 11+ messages in thread From: Thomas Huth @ 2020-09-09 7:20 UTC (permalink / raw) To: chen huacai Cc: Laurent Vivier, Peter Maydell, Aleksandar Rikalo, Huacai Chen, Philippe Mathieu-Daudé, qemu-level, Aleksandar Markovic, Paolo Bonzini, David Gibson, Huacai Chen, Aurelien Jarno 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... Thanks, Thomas ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS 2020-09-09 7:20 ` Thomas Huth @ 2020-09-10 1:31 ` Huacai Chen 0 siblings, 0 replies; 11+ messages in thread From: Huacai Chen @ 2020-09-10 1:31 UTC (permalink / raw) To: Thomas Huth Cc: Laurent Vivier, chen huacai, Aleksandar Rikalo, Peter Maydell, Philippe Mathieu-Daudé, qemu-level, Aleksandar Markovic, Paolo Bonzini, David Gibson, Aurelien Jarno 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-09-10 1:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-24 8:11 [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS Huacai Chen 2020-09-02 13:55 ` Philippe Mathieu-Daudé 2020-09-03 0:58 ` Huacai Chen 2020-09-07 3:39 ` Philippe Mathieu-Daudé 2020-09-08 0:41 ` Huacai Chen 2020-09-08 17:25 ` Thomas Huth 2020-09-08 17:59 ` Eduardo Habkost 2020-09-08 18:12 ` Philippe Mathieu-Daudé 2020-09-09 2:57 ` chen huacai 2020-09-09 7:20 ` Thomas Huth 2020-09-10 1:31 ` Huacai Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.