Hi, all, On Wed, Sep 9, 2020 at 1:25 AM Thomas Huth <th...@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.de...@gmail.com> > > Signed-off-by: Huacai Chen <che...@lemote.com> > > Co-developed-by: Jiaxun Yang <jiaxun.y...@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