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

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

[Huacai Chen]

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

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

[Philippe Mathieu-Daudé]

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

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

[Huacai Chen]

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

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

[Philippe Mathieu-Daudé]

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

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

[Huacai Chen]

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

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

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

[Eduardo Habkost]

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

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

[Philippe Mathieu-Daudé]

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

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

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

[Thomas Huth]

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

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

[Huacai Chen]

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
>