[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Daniel Henrique Barboza
Subject: Re: [PATCH v2 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
Date: Thu, 10 Dec 2020 11:19:59 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0

On 12/10/20 11:14 AM, Greg Kurz wrote:
On Thu, 10 Dec 2020 15:01:20 +0100
Greg Kurz <groug@kaod.org> wrote:

On Thu, 10 Dec 2020 10:07:21 -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>
  hw/ppc/spapr.c | 15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..f097f4ea30 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3021,17 +3021,18 @@ static void spapr_machine_init(MachineState *machine)
+#define DEFAULT_KVM_TYPE "auto"
  static int spapr_kvm_type(MachineState *machine, const char *vm_type)
-    if (!vm_type) {
+    if (!g_ascii_strcasecmp(vm_type, DEFAULT_KVM_TYPE)) {

strcmp() would be better here : we don't want to support an
already existing "AUTO" value like for the other ones.

And you should also keep the check on vm_type since it is NULL
if the kvm-type wasn't passed to the command line:

qemu-system-ppc64: GLib: g_ascii_strcasecmp: assertion 's1 != NULL' failed

Hm, I tested this case and didn't hit that. Perhaphs because I am testing this
on top of Paolo's patches instead of master.

I'll keep the NULL check as a fallback to 'auto' just to be safe then.


And, of course, add a comment to explain why the two other
ones are handled differently.

          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;
@@ -3131,7 +3132,7 @@ static char *spapr_get_kvm_type(Object *obj, Error **errp)
      SpaprMachineState *spapr = SPAPR_MACHINE(obj);
- return g_strdup(spapr->kvm_type);
+    return g_strdup(spapr->kvm_type ? spapr->kvm_type : DEFAULT_KVM_TYPE);

As in Paolo's diff, it seems better to simply initialize spapr->kvm_type ...

static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp)
@@ -3273,7 +3274,11 @@ static void spapr_instance_init(Object *obj)

... here.

     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, 
+                                    "Specifies the KVM virtualization mode 
+                                    " hv, pr). Defaults to 'auto'. This mode will 
+                                    " any available KVM module loaded in the 
+                                    " where kvm_hv takes precedence if both kvm_hv 
+                                    " kvm_pr are loaded.");
      object_property_add_bool(obj, "modern-hotplug-events",

reply via email to

[Prev in Thread] Current Thread [Next in Thread]