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

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

[Daniel Henrique Barboza]

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: "HV", "PR" and, if no kvm-type was set by the user,
DEFAULT_KVM_TYPE. This allows us 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 what
happens if the user does not set this option. This information 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>
---

The case I mentioned in the second paragraph is happening when we try to
execute a pseries guest with '--enable-kvm' using Paolo's patch 
"vl: make qemu_get_machine_opts static" [1]:

$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
qemu-system-ppc64: Unknown kvm-type specified '' 

This happens due to the differences between how qemu_opt_get() and
object_property_get_str() works when there is no 'kvm-type' specified.
See [2] for more info about the issue found.


[1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html 


 hw/ppc/spapr.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..32d938dc6a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3021,9 +3021,10 @@ 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) {
+    if (!strcmp(vm_type, DEFAULT_KVM_TYPE)) {
         return 0;
     }
 
@@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(obj);
 
+    /*
+     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
+     * instead of NULL. This allows us to be more predictable with what
+     * is expected to happen in spapr_kvm_type(), since we can stop relying
+     * on receiving a 'NULL' parameter as a valid input there.
+     */
+    if (!spapr->kvm_type) {
+        return g_strdup(DEFAULT_KVM_TYPE);
+    }
+
     return g_strdup(spapr->kvm_type);
 }
 
@@ -3273,7 +3284,11 @@ static void spapr_instance_init(Object *obj)
     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 (HV, PR)."
+                                    " If not specified, defaults to any available KVM"
+                                    " module loaded in the host. In case both kvm_hv"
+                                    " and kvm_pr are loaded, kvm_hv takes precedence.");
+
     object_property_add_bool(obj, "modern-hotplug-events",
                             spapr_get_modern_hotplug_events,
                             spapr_set_modern_hotplug_events);
-- 
2.26.2

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

[Greg Kurz]

Hi Daniel,

On Tue,  8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user,
> DEFAULT_KVM_TYPE. This allows us 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 what
> happens if the user does not set this option. This information 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>
> ---
> 
> The case I mentioned in the second paragraph is happening when we try to
> execute a pseries guest with '--enable-kvm' using Paolo's patch 
> "vl: make qemu_get_machine_opts static" [1]:
> 
> $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
> qemu-system-ppc64: Unknown kvm-type specified '' 
> 
> This happens due to the differences between how qemu_opt_get() and
> object_property_get_str() works when there is no 'kvm-type' specified.
> See [2] for more info about the issue found.
> 
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html 
> 
> 
>  hw/ppc/spapr.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..32d938dc6a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3021,9 +3021,10 @@ 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) {
> +    if (!strcmp(vm_type, DEFAULT_KVM_TYPE)) {
>          return 0;
>      }
>  
> @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>  
> +    /*
> +     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
> +     * instead of NULL. This allows us to be more predictable with what
> +     * is expected to happen in spapr_kvm_type(), since we can stop relying
> +     * on receiving a 'NULL' parameter as a valid input there.
> +     */

Returning what is obviously a default value is straightforward enough
that is doesn't need to a comment IMHO.

> +    if (!spapr->kvm_type) {
> +        return g_strdup(DEFAULT_KVM_TYPE);
> +    }
> +
>      return g_strdup(spapr->kvm_type);

Alternatively you could simply do:

    return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE);

>  }
>  
> @@ -3273,7 +3284,11 @@ static void spapr_instance_init(Object *obj)
>      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 (HV, PR)."

Why not documenting "auto" as a valid option as well ?

While here you could maybe convert HV and PR to lowercase in order to
have a prettier "hv, pr, auto" set of valid values in the help string.
You'd need to convert the related checks in spapr_kvm_type() to still
check for the uppercase versions for compatibility, eg. by using
g_ascii_strcasecmp().

> +                                    " If not specified, defaults to any available KVM"
> +                                    " module loaded in the host. In case both kvm_hv"
> +                                    " and kvm_pr are loaded, kvm_hv takes precedence.");
> +

s/If not specified/If 'auto'/ and mention 'auto' as being the default value.

>      object_property_add_bool(obj, "modern-hotplug-events",
>                              spapr_get_modern_hotplug_events,
>                              spapr_set_modern_hotplug_events);

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

[Daniel Henrique Barboza]

On 12/8/20 11:33 AM, Greg Kurz wrote:
> Hi Daniel,
> 
> On Tue,  8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user,
>> DEFAULT_KVM_TYPE. This allows us 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 what
>> happens if the user does not set this option. This information 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>
>> ---
>>
>> The case I mentioned in the second paragraph is happening when we try to
>> execute a pseries guest with '--enable-kvm' using Paolo's patch
>> "vl: make qemu_get_machine_opts static" [1]:
>>
>> $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
>> qemu-system-ppc64: Unknown kvm-type specified ''
>>
>> This happens due to the differences between how qemu_opt_get() and
>> object_property_get_str() works when there is no 'kvm-type' specified.
>> See [2] for more info about the issue found.
>>
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
>> [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html
>>
>>
>>   hw/ppc/spapr.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index b7e0894019..32d938dc6a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3021,9 +3021,10 @@ 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) {
>> +    if (!strcmp(vm_type, DEFAULT_KVM_TYPE)) {
>>           return 0;
>>       }
>>   
>> @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp)
>>   {
>>       SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>>   
>> +    /*
>> +     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
>> +     * instead of NULL. This allows us to be more predictable with what
>> +     * is expected to happen in spapr_kvm_type(), since we can stop relying
>> +     * on receiving a 'NULL' parameter as a valid input there.
>> +     */
> 
> Returning what is obviously a default value is straightforward enough
> that is doesn't need to a comment IMHO.
> 
>> +    if (!spapr->kvm_type) {
>> +        return g_strdup(DEFAULT_KVM_TYPE);
>> +    }
>> +
>>       return g_strdup(spapr->kvm_type);
> 
> Alternatively you could simply do:
> 
>      return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE);

Got it. I'll update it in v2.

> 
>>   }
>>   
>> @@ -3273,7 +3284,11 @@ static void spapr_instance_init(Object *obj)
>>       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 (HV, PR)."
> 
> Why not documenting "auto" as a valid option as well ?


This was my first idea but I got cold feet about it. I got afraid that
exposing the default 'auto' option might misled users into believing that
we're adding a new configuration option, even thought it's just the
same behavior users are dealing with for 7+ years. I chose to use the
'auto' value as an internal default that isn't documented . Granted, this
also means that we have now a hidden 'kvm-type=auto' setting, so yeah.

If there is a consensus that exposing the "auto" default is the right
thing to do in this case I'll just go ahead and expose it.



> 
> While here you could maybe convert HV and PR to lowercase in order to
> have a prettier "hv, pr, auto" set of valid values in the help string.
> You'd need to convert the related checks in spapr_kvm_type() to still
> check for the uppercase versions for compatibility, eg. by using
> g_ascii_strcasecmp().

Roger that.

> 
>> +                                    " If not specified, defaults to any available KVM"
>> +                                    " module loaded in the host. In case both kvm_hv"
>> +                                    " and kvm_pr are loaded, kvm_hv takes precedence.");
>> +
> 
> s/If not specified/If 'auto'/ and mention 'auto' as being the default value.


As I said above, I'll happily mention the 'auto' default if we can agree
that this will not lead to user confusion thinking this is a new option and
so on.


Thanks,


DHB

> 
>>       object_property_add_bool(obj, "modern-hotplug-events",
>>                               spapr_get_modern_hotplug_events,
>>                               spapr_set_modern_hotplug_events);
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6661 bytes --]

On Tue, Dec 08, 2020 at 12:32:46PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 12/8/20 11:33 AM, Greg Kurz wrote:
> > Hi Daniel,
> > 
> > On Tue,  8 Dec 2020 10:45:36 -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: "HV", "PR" and, if no kvm-type was set by the user,
> > > DEFAULT_KVM_TYPE. This allows us 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 what
> > > happens if the user does not set this option. This information 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>
> > > ---
> > > 
> > > The case I mentioned in the second paragraph is happening when we try to
> > > execute a pseries guest with '--enable-kvm' using Paolo's patch
> > > "vl: make qemu_get_machine_opts static" [1]:
> > > 
> > > $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
> > > qemu-system-ppc64: Unknown kvm-type specified ''
> > > 
> > > This happens due to the differences between how qemu_opt_get() and
> > > object_property_get_str() works when there is no 'kvm-type' specified.
> > > See [2] for more info about the issue found.
> > > 
> > > 
> > > [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
> > > [2] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01578.html
> > > 
> > > 
> > >   hw/ppc/spapr.c | 19 +++++++++++++++++--
> > >   1 file changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index b7e0894019..32d938dc6a 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3021,9 +3021,10 @@ 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) {
> > > +    if (!strcmp(vm_type, DEFAULT_KVM_TYPE)) {
> > >           return 0;
> > >       }
> > > @@ -3131,6 +3132,16 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp)
> > >   {
> > >       SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +    /*
> > > +     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
> > > +     * instead of NULL. This allows us to be more predictable with what
> > > +     * is expected to happen in spapr_kvm_type(), since we can stop relying
> > > +     * on receiving a 'NULL' parameter as a valid input there.
> > > +     */
> > 
> > Returning what is obviously a default value is straightforward enough
> > that is doesn't need to a comment IMHO.
> > 
> > > +    if (!spapr->kvm_type) {
> > > +        return g_strdup(DEFAULT_KVM_TYPE);
> > > +    }
> > > +
> > >       return g_strdup(spapr->kvm_type);
> > 
> > Alternatively you could simply do:
> > 
> >      return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE);
> 
> Got it. I'll update it in v2.
> 
> > 
> > >   }
> > > @@ -3273,7 +3284,11 @@ static void spapr_instance_init(Object *obj)
> > >       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 (HV, PR)."
> > 
> > Why not documenting "auto" as a valid option as well ?
> 
> 
> This was my first idea but I got cold feet about it. I got afraid that
> exposing the default 'auto' option might misled users into believing that
> we're adding a new configuration option, even thought it's just the
> same behavior users are dealing with for 7+ years. I chose to use the
> 'auto' value as an internal default that isn't documented . Granted, this
> also means that we have now a hidden 'kvm-type=auto' setting, so yeah.
> 
> If there is a consensus that exposing the "auto" default is the right
> thing to do in this case I'll just go ahead and expose it.

I think it should be fine to expose it.

> > While here you could maybe convert HV and PR to lowercase in order to
> > have a prettier "hv, pr, auto" set of valid values in the help string.
> > You'd need to convert the related checks in spapr_kvm_type() to still
> > check for the uppercase versions for compatibility, eg. by using
> > g_ascii_strcasecmp().
> 
> Roger that.
> 
> > 
> > > +                                    " If not specified, defaults to any available KVM"
> > > +                                    " module loaded in the host. In case both kvm_hv"
> > > +                                    " and kvm_pr are loaded, kvm_hv takes precedence.");
> > > +
> > 
> > s/If not specified/If 'auto'/ and mention 'auto' as being the default value.
> 
> 
> As I said above, I'll happily mention the 'auto' default if we can agree
> that this will not lead to user confusion thinking this is a new option and
> so on.
> 
> 
> Thanks,
> 
> 
> DHB
> 
> > 
> > >       object_property_add_bool(obj, "modern-hotplug-events",
> > >                               spapr_get_modern_hotplug_events,
> > >                               spapr_set_modern_hotplug_events);
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paolo Bonzini]

To sum up everything:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2d5aeeb45a..61f0963916 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine, 
const char *vm_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;
      }

@@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj, 
Error **errp)
  {
      SpaprMachineState *spapr = SPAPR_MACHINE(obj);

-    /*
-     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
-     * instead of NULL. This allows us to be more predictable with what
-     * is expected to happen in spapr_kvm_type(), since we can stop relying
-     * on receiving a 'NULL' parameter as a valid input there.
-     */
-    if (!spapr->kvm_type) {
-        return g_strdup(DEFAULT_KVM_TYPE);
-    }
-
      return g_strdup(spapr->kvm_type);
  }

@@ -3294,11 +3284,13 @@ 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)."
-                                    " If not specified, defaults to any 
available KVM"
+                                    "Specifies the KVM virtualization 
mode (hv, pr, auto)."
+                                    " auto is the default and allows 
any available KVM"
                                      " module loaded in the host. In 
case both kvm_hv"
                                      " and kvm_pr are loaded, kvm_hv 
takes precedence.");

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

[Greg Kurz]

On Thu, 10 Dec 2020 13:34:59 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> To sum up everything:
> 

LGTM

> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2d5aeeb45a..61f0963916 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine, 
> const char *vm_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;
>       }
> 
> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj, 
> Error **errp)
>   {
>       SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> 
> -    /*
> -     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
> -     * instead of NULL. This allows us to be more predictable with what
> -     * is expected to happen in spapr_kvm_type(), since we can stop relying
> -     * on receiving a 'NULL' parameter as a valid input there.
> -     */
> -    if (!spapr->kvm_type) {
> -        return g_strdup(DEFAULT_KVM_TYPE);
> -    }
> -
>       return g_strdup(spapr->kvm_type);
>   }
> 
> @@ -3294,11 +3284,13 @@ 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)."
> -                                    " If not specified, defaults to any 
> available KVM"
> +                                    "Specifies the KVM virtualization 
> mode (hv, pr, auto)."
> +                                    " auto is the default and allows 
> any available KVM"
>                                       " module loaded in the host. In 
> case both kvm_hv"
>                                       " and kvm_pr are loaded, kvm_hv 
> takes precedence.");
> 

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

[Daniel Henrique Barboza]

On 12/10/20 9:47 AM, Greg Kurz wrote:
> On Thu, 10 Dec 2020 13:34:59 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> To sum up everything:
>>
> 
> LGTM

I just sent a v2 with a bit more done (e.g. added ignore case compare
for 'auto'). Feel free to use that version or this one amended by this
diff from Paolo.


Thanks,


DHB

> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2d5aeeb45a..61f0963916 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine,
>> const char *vm_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;
>>        }
>>
>> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj,
>> Error **errp)
>>    {
>>        SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>>
>> -    /*
>> -     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
>> -     * instead of NULL. This allows us to be more predictable with what
>> -     * is expected to happen in spapr_kvm_type(), since we can stop relying
>> -     * on receiving a 'NULL' parameter as a valid input there.
>> -     */
>> -    if (!spapr->kvm_type) {
>> -        return g_strdup(DEFAULT_KVM_TYPE);
>> -    }
>> -
>>        return g_strdup(spapr->kvm_type);
>>    }
>>
>> @@ -3294,11 +3284,13 @@ 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)."
>> -                                    " If not specified, defaults to any
>> available KVM"
>> +                                    "Specifies the KVM virtualization
>> mode (hv, pr, auto)."
>> +                                    " auto is the default and allows
>> any available KVM"
>>                                        " module loaded in the host. In
>> case both kvm_hv"
>>                                        " and kvm_pr are loaded, kvm_hv
>> takes precedence.");
>>
> 

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

[Greg Kurz]

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

> 
> 
> On 12/10/20 9:47 AM, Greg Kurz wrote:
> > On Thu, 10 Dec 2020 13:34:59 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> To sum up everything:
> >>
> > 
> > LGTM
> 
> I just sent a v2 with a bit more done (e.g. added ignore case compare
> for 'auto'). Feel free to use that version or this one amended by this
> diff from Paolo.
> 

The "bit more done" in your v2 is very important as it prevents
spapr_kvm_type() to exit(1) if "auto" is explicitly passed on
the command line. So I think we should go ahead with yours.

> 
> Thanks,
> 
> 
> DHB
> 
> > 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 2d5aeeb45a..61f0963916 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine,
> >> const char *vm_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;
> >>        }
> >>
> >> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj,
> >> Error **errp)
> >>    {
> >>        SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> >>
> >> -    /*
> >> -     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
> >> -     * instead of NULL. This allows us to be more predictable with what
> >> -     * is expected to happen in spapr_kvm_type(), since we can stop relying
> >> -     * on receiving a 'NULL' parameter as a valid input there.
> >> -     */
> >> -    if (!spapr->kvm_type) {
> >> -        return g_strdup(DEFAULT_KVM_TYPE);
> >> -    }
> >> -
> >>        return g_strdup(spapr->kvm_type);
> >>    }
> >>
> >> @@ -3294,11 +3284,13 @@ 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)."
> >> -                                    " If not specified, defaults to any
> >> available KVM"
> >> +                                    "Specifies the KVM virtualization
> >> mode (hv, pr, auto)."
> >> +                                    " auto is the default and allows
> >> any available KVM"
> >>                                        " module loaded in the host. In
> >> case both kvm_hv"
> >>                                        " and kvm_pr are loaded, kvm_hv
> >> takes precedence.");
> >>
> >