Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS

[Thomas Huth]

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)
>