Re: [PATCH v3 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL

[Greg Kurz]

On Thu, 10 Dec 2020 11:55:17 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where
> the function returns 0. This is relying on the current QEMU machine
> options handling logic, where the absence of the 'kvm-type' option
> will be reflected as 'vm_type=NULL' in this function.
> 
> This is not robust, and will break if QEMU options code decides to propagate
> something else in the case mentioned above (e.g. an empty string instead
> of NULL).
> 
> Let's avoid this entirely by setting a non-NULL default value in case of
> no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed
> values of kvm-type: "auto", "hv", and "pr", with "auto" being the default
> if no kvm-type was set by the user. This allows us to always be predictable
> regardless of any enhancements/changes made in QEMU options mechanics.
> 
> While we're at it, let's also document in 'kvm-type' description the
> already existing default mode, now named 'auto'. The information provided
> about it is based on how the pseries kernel handles the KVM_CREATE_VM
> ioctl(), where the default value '0' makes the kernel choose an available
> KVM module to use, giving precedence to kvm_hv. This logic is described in
> the kernel source file arch/powerpc/kvm/powerpc.c, function 
> kvm_arch_init_vm().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..877bd264ce 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3021,17 +3021,25 @@ static void spapr_machine_init(MachineState *machine)
>      qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond);
>  }
>  
> +#define DEFAULT_KVM_TYPE "auto"
>  static int spapr_kvm_type(MachineState *machine, const char *vm_type)
>  {
> -    if (!vm_type) {
> +    /*
> +     * The use of g_ascii_strcasecmp() for 'hv' and 'pr' is to
> +     * accomodate the 'HV' and 'PV' formats that exists in the
> +     * wild. The 'auto' mode is being introduced already as
> +     * lower-case, thus we don't need to bother checking for
> +     * "AUTO".
> +     */
> +    if (!vm_type || !strcmp(vm_type, DEFAULT_KVM_TYPE)) {
>          return 0;
>      }
>  
> -    if (!strcmp(vm_type, "HV")) {
> +    if (!g_ascii_strcasecmp(vm_type, "hv")) {
>          return 1;
>      }
>  
> -    if (!strcmp(vm_type, "PR")) {
> +    if (!g_ascii_strcasecmp(vm_type, "pr")) {
>          return 2;
>      }
>  
> @@ -3270,10 +3278,15 @@ static void spapr_instance_init(Object *obj)
>  
>      spapr->htab_fd = -1;
>      spapr->use_hotplug_event_source = true;
> +    spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE);
>      object_property_add_str(obj, "kvm-type",
>                              spapr_get_kvm_type, spapr_set_kvm_type);
>      object_property_set_description(obj, "kvm-type",
> -                                    "Specifies the KVM virtualization mode 
> (HV, PR)");
> +                                    "Specifies the KVM virtualization mode 
> (auto,"
> +                                    " hv, pr). Defaults to 'auto'. This mode 
> will use"
> +                                    " any available KVM module loaded in the 
> host,"
> +                                    " where kvm_hv takes precedence if both 
> kvm_hv and"
> +                                    " kvm_pr are loaded.");
>      object_property_add_bool(obj, "modern-hotplug-events",
>                              spapr_get_modern_hotplug_events,
>                              spapr_set_modern_hotplug_events);