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

[chen huacai]

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