All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function
  2024-08-09  4:51 [PATCH 0/2] Some refactoring Ani Sinha
@ 2024-08-09  4:51 ` Ani Sinha
  0 siblings, 0 replies; 8+ messages in thread
From: Ani Sinha @ 2024-08-09  4:51 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Ani Sinha, zhao1.liu, cfontana, qemu-trivial, kvm, qemu-devel

Refactoring the core logic around KVM_CREATE_VM into its own separate function
so that it can be called from other functions in subsequent patches. There is
no functional change in this patch.

CC: pbonzini@redhat.com
CC: zhao1.liu@intel.com
CC: cfontana@suse.de
CC: qemu-trivial@nongnu.org
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 accel/kvm/kvm-all.c | 88 +++++++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 34 deletions(-)

changelog:
v2: s/fprintf/warn_report as suggested by zhao
v3: s/warn_report/error_report. function names adjusted to conform to
other names. fprintf -> error_report() moved to its own patch.

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 899b5264e3..610b3ead32 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2385,6 +2385,57 @@ uint32_t kvm_dirty_ring_size(void)
     return kvm_state->kvm_dirty_ring_size;
 }
 
+static int kvm_create_vm(MachineState *ms, KVMState *s, int type)
+{
+    int ret;
+
+    do {
+        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
+    } while (ret == -EINTR);
+
+    if (ret < 0) {
+        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
+                    strerror(-ret));
+
+#ifdef TARGET_S390X
+        if (ret == -EINVAL) {
+            error_report("Host kernel setup problem detected. Please verify:");
+            error_report("- for kernels supporting the switch_amode or"
+                        " user_mode parameters, whether");
+            error_report("  user space is running in primary address space");
+            error_report("- for kernels supporting the vm.allocate_pgste "
+                        "sysctl, whether it is enabled");
+        }
+#elif defined(TARGET_PPC)
+        if (ret == -EINVAL) {
+            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
+                        (type == 2) ? "pr" : "hv");
+        }
+#endif
+    }
+
+    return ret;
+}
+
+static int kvm_machine_type(MachineState *ms)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    int type;
+
+    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+        g_autofree char *kvm_type;
+        kvm_type = object_property_get_str(OBJECT(current_machine),
+                                           "kvm-type",
+                                           &error_abort);
+        type = mc->kvm_type(ms, kvm_type);
+    } else if (mc->kvm_type) {
+        type = mc->kvm_type(ms, NULL);
+    } else {
+        type = kvm_arch_get_default_type(ms);
+    }
+    return type;
+}
+
 static int kvm_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2467,47 +2518,16 @@ static int kvm_init(MachineState *ms)
     }
     s->as = g_new0(struct KVMAs, s->nr_as);
 
-    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
-        g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine),
-                                                            "kvm-type",
-                                                            &error_abort);
-        type = mc->kvm_type(ms, kvm_type);
-    } else if (mc->kvm_type) {
-        type = mc->kvm_type(ms, NULL);
-    } else {
-        type = kvm_arch_get_default_type(ms);
-    }
-
+    type = kvm_machine_type(ms);
     if (type < 0) {
         ret = -EINVAL;
         goto err;
     }
 
-    do {
-        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
-    } while (ret == -EINTR);
-
+    ret = kvm_create_vm(ms, s, type);
     if (ret < 0) {
-        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
-                    strerror(-ret));
-
-#ifdef TARGET_S390X
-        if (ret == -EINVAL) {
-            error_report("Host kernel setup problem detected. Please verify:");
-            error_report("- for kernels supporting the switch_amode or"
-                        " user_mode parameters, whether");
-            error_report("  user space is running in primary address space");
-            error_report("- for kernels supporting the vm.allocate_pgste "
-                        "sysctl, whether it is enabled");
-        }
-#elif defined(TARGET_PPC)
-        if (ret == -EINVAL) {
-            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
-                        (type == 2) ? "pr" : "hv");
-        }
-#endif
-    }
         goto err;
+    }
 
     s->vmfd = ret;
 
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 0/2] Some refactoring
@ 2024-08-09  5:10 Ani Sinha
  2024-08-09  5:10 ` [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions Ani Sinha
  2024-08-09  5:10 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha
  0 siblings, 2 replies; 8+ messages in thread
From: Ani Sinha @ 2024-08-09  5:10 UTC (permalink / raw)
  Cc: Ani Sinha, qemu-trivial, qemu-devel, zhao1.liu, pbonzini

replace fprintf() with error_report() in kvm_init()
refactor code in kvm_init() to move core vm creation operation to its
own function.

CC: qemu-trivial@nongnu.org
CC: qemu-devel@nongnu.org
CC: zhao1.liu@intel.com
CC: pbonzini@redhat.com


Ani Sinha (2):
  kvm: replace fprintf with error_report() in kvm_init() for error
    conditions
  kvm: refactor core virtual machine creation into its own function

 accel/kvm/kvm-all.c | 106 +++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 45 deletions(-)

-- 
2.45.2



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions
  2024-08-09  5:10 [PATCH 0/2] Some refactoring Ani Sinha
@ 2024-08-09  5:10 ` Ani Sinha
  2024-08-09  6:13   ` Zhao Liu
  2024-08-09  5:10 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha
  1 sibling, 1 reply; 8+ messages in thread
From: Ani Sinha @ 2024-08-09  5:10 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Ani Sinha, qemu-trivial, zhao1.liu, kvm, qemu-devel

error_report() is more appropriate for error situations. Replace fprintf with
error_report. Cosmetic. No functional change.

CC: qemu-trivial@nongnu.org
CC: zhao1.liu@intel.com
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 accel/kvm/kvm-all.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

changelog:
v2: fix a bug.

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 75d11a07b2..ac168b9663 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2427,7 +2427,7 @@ static int kvm_init(MachineState *ms)
     QLIST_INIT(&s->kvm_parked_vcpus);
     s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
     if (s->fd == -1) {
-        fprintf(stderr, "Could not access KVM kernel module: %m\n");
+        error_report("Could not access KVM kernel module: %m");
         ret = -errno;
         goto err;
     }
@@ -2437,13 +2437,13 @@ static int kvm_init(MachineState *ms)
         if (ret >= 0) {
             ret = -EINVAL;
         }
-        fprintf(stderr, "kvm version too old\n");
+        error_report("kvm version too old");
         goto err;
     }
 
     if (ret > KVM_API_VERSION) {
         ret = -EINVAL;
-        fprintf(stderr, "kvm version not supported\n");
+        error_report("kvm version not supported");
         goto err;
     }
 
@@ -2488,26 +2488,22 @@ static int kvm_init(MachineState *ms)
     } while (ret == -EINTR);
 
     if (ret < 0) {
-        fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret,
-                strerror(-ret));
+        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
+                    strerror(-ret));
 
 #ifdef TARGET_S390X
         if (ret == -EINVAL) {
-            fprintf(stderr,
-                    "Host kernel setup problem detected. Please verify:\n");
-            fprintf(stderr, "- for kernels supporting the switch_amode or"
-                    " user_mode parameters, whether\n");
-            fprintf(stderr,
-                    "  user space is running in primary address space\n");
-            fprintf(stderr,
-                    "- for kernels supporting the vm.allocate_pgste sysctl, "
-                    "whether it is enabled\n");
+            error_report("Host kernel setup problem detected. Please verify:");
+            error_report("- for kernels supporting the switch_amode or"
+                        " user_mode parameters, whether");
+            error_report("  user space is running in primary address space");
+            error_report("- for kernels supporting the vm.allocate_pgste "
+                        "sysctl, whether it is enabled");
         }
 #elif defined(TARGET_PPC)
         if (ret == -EINVAL) {
-            fprintf(stderr,
-                    "PPC KVM module is not loaded. Try modprobe kvm_%s.\n",
-                    (type == 2) ? "pr" : "hv");
+            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
+                        (type == 2) ? "pr" : "hv");
         }
 #endif
         goto err;
@@ -2526,9 +2522,9 @@ static int kvm_init(MachineState *ms)
                         nc->name, nc->num, soft_vcpus_limit);
 
             if (nc->num > hard_vcpus_limit) {
-                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
-                        "the maximum cpus supported by KVM (%d)\n",
-                        nc->name, nc->num, hard_vcpus_limit);
+                error_report("Number of %s cpus requested (%d) exceeds "
+                             "the maximum cpus supported by KVM (%d)",
+                             nc->name, nc->num, hard_vcpus_limit);
                 exit(1);
             }
         }
@@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms)
     }
     if (missing_cap) {
         ret = -EINVAL;
-        fprintf(stderr, "kvm does not support %s\n%s",
-                missing_cap->name, upgrade_note);
+        error_report("kvm does not support %s", missing_cap->name);
+        error_report("%s", upgrade_note);
         goto err;
     }
 
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function
  2024-08-09  5:10 [PATCH 0/2] Some refactoring Ani Sinha
  2024-08-09  5:10 ` [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions Ani Sinha
@ 2024-08-09  5:10 ` Ani Sinha
  2024-08-09  5:31   ` Zhao Liu
  2024-08-09  7:58   ` Claudio Fontana
  1 sibling, 2 replies; 8+ messages in thread
From: Ani Sinha @ 2024-08-09  5:10 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Ani Sinha, zhao1.liu, cfontana, qemu-trivial, kvm, qemu-devel

Refactoring the core logic around KVM_CREATE_VM into its own separate function
so that it can be called from other functions in subsequent patches. There is
no functional change in this patch.

CC: pbonzini@redhat.com
CC: zhao1.liu@intel.com
CC: cfontana@suse.de
CC: qemu-trivial@nongnu.org
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 accel/kvm/kvm-all.c | 86 ++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 33 deletions(-)

changelog:
v2: s/fprintf/warn_report as suggested by zhao
v3: s/warn_report/error_report. function names adjusted to conform to
other names. fprintf -> error_report() moved to its own patch.

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ac168b9663..610b3ead32 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2385,6 +2385,57 @@ uint32_t kvm_dirty_ring_size(void)
     return kvm_state->kvm_dirty_ring_size;
 }
 
+static int kvm_create_vm(MachineState *ms, KVMState *s, int type)
+{
+    int ret;
+
+    do {
+        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
+    } while (ret == -EINTR);
+
+    if (ret < 0) {
+        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
+                    strerror(-ret));
+
+#ifdef TARGET_S390X
+        if (ret == -EINVAL) {
+            error_report("Host kernel setup problem detected. Please verify:");
+            error_report("- for kernels supporting the switch_amode or"
+                        " user_mode parameters, whether");
+            error_report("  user space is running in primary address space");
+            error_report("- for kernels supporting the vm.allocate_pgste "
+                        "sysctl, whether it is enabled");
+        }
+#elif defined(TARGET_PPC)
+        if (ret == -EINVAL) {
+            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
+                        (type == 2) ? "pr" : "hv");
+        }
+#endif
+    }
+
+    return ret;
+}
+
+static int kvm_machine_type(MachineState *ms)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    int type;
+
+    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+        g_autofree char *kvm_type;
+        kvm_type = object_property_get_str(OBJECT(current_machine),
+                                           "kvm-type",
+                                           &error_abort);
+        type = mc->kvm_type(ms, kvm_type);
+    } else if (mc->kvm_type) {
+        type = mc->kvm_type(ms, NULL);
+    } else {
+        type = kvm_arch_get_default_type(ms);
+    }
+    return type;
+}
+
 static int kvm_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2467,45 +2518,14 @@ static int kvm_init(MachineState *ms)
     }
     s->as = g_new0(struct KVMAs, s->nr_as);
 
-    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
-        g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine),
-                                                            "kvm-type",
-                                                            &error_abort);
-        type = mc->kvm_type(ms, kvm_type);
-    } else if (mc->kvm_type) {
-        type = mc->kvm_type(ms, NULL);
-    } else {
-        type = kvm_arch_get_default_type(ms);
-    }
-
+    type = kvm_machine_type(ms);
     if (type < 0) {
         ret = -EINVAL;
         goto err;
     }
 
-    do {
-        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
-    } while (ret == -EINTR);
-
+    ret = kvm_create_vm(ms, s, type);
     if (ret < 0) {
-        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
-                    strerror(-ret));
-
-#ifdef TARGET_S390X
-        if (ret == -EINVAL) {
-            error_report("Host kernel setup problem detected. Please verify:");
-            error_report("- for kernels supporting the switch_amode or"
-                        " user_mode parameters, whether");
-            error_report("  user space is running in primary address space");
-            error_report("- for kernels supporting the vm.allocate_pgste "
-                        "sysctl, whether it is enabled");
-        }
-#elif defined(TARGET_PPC)
-        if (ret == -EINVAL) {
-            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
-                        (type == 2) ? "pr" : "hv");
-        }
-#endif
         goto err;
     }
 
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function
  2024-08-09  5:10 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha
@ 2024-08-09  5:31   ` Zhao Liu
  2024-08-09  7:58   ` Claudio Fontana
  1 sibling, 0 replies; 8+ messages in thread
From: Zhao Liu @ 2024-08-09  5:31 UTC (permalink / raw)
  To: Ani Sinha; +Cc: Paolo Bonzini, cfontana, qemu-trivial, kvm, qemu-devel

On Fri, Aug 09, 2024 at 10:40:54AM +0530, Ani Sinha wrote:
> Date: Fri,  9 Aug 2024 10:40:54 +0530
> From: Ani Sinha <anisinha@redhat.com>
> Subject: [PATCH v3 2/2] kvm: refactor core virtual machine creation into
>  its own function
> X-Mailer: git-send-email 2.45.2
> 
> Refactoring the core logic around KVM_CREATE_VM into its own separate function
> so that it can be called from other functions in subsequent patches. There is
> no functional change in this patch.
> 
> CC: pbonzini@redhat.com
> CC: zhao1.liu@intel.com
> CC: cfontana@suse.de
> CC: qemu-trivial@nongnu.org
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 86 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 53 insertions(+), 33 deletions(-)
> 
> changelog:
> v2: s/fprintf/warn_report as suggested by zhao
> v3: s/warn_report/error_report. function names adjusted to conform to
> other names. fprintf -> error_report() moved to its own patch.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions
  2024-08-09  5:10 ` [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions Ani Sinha
@ 2024-08-09  6:13   ` Zhao Liu
  2024-08-09  7:06     ` Ani Sinha
  0 siblings, 1 reply; 8+ messages in thread
From: Zhao Liu @ 2024-08-09  6:13 UTC (permalink / raw)
  To: Ani Sinha; +Cc: Paolo Bonzini, qemu-trivial, kvm, qemu-devel

On Fri, Aug 09, 2024 at 10:40:53AM +0530, Ani Sinha wrote:
> Date: Fri,  9 Aug 2024 10:40:53 +0530
> From: Ani Sinha <anisinha@redhat.com>
> Subject: [PATCH v2 1/2] kvm: replace fprintf with error_report() in
>  kvm_init() for error conditions
> X-Mailer: git-send-email 2.45.2
> 
> error_report() is more appropriate for error situations. Replace fprintf with
> error_report. Cosmetic. No functional change.
> 
> CC: qemu-trivial@nongnu.org
> CC: zhao1.liu@intel.com
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 
> changelog:
> v2: fix a bug.

Generally good to me. Only some nits below, otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

>  #ifdef TARGET_S390X
>          if (ret == -EINVAL) {
> -            fprintf(stderr,
> -                    "Host kernel setup problem detected. Please verify:\n");
> -            fprintf(stderr, "- for kernels supporting the switch_amode or"
> -                    " user_mode parameters, whether\n");
> -            fprintf(stderr,
> -                    "  user space is running in primary address space\n");
> -            fprintf(stderr,
> -                    "- for kernels supporting the vm.allocate_pgste sysctl, "
> -                    "whether it is enabled\n");
> +            error_report("Host kernel setup problem detected. Please verify:");

The doc of error_report() said it doesn't want multiple sentences or trailing
punctuation:

"The resulting message should be a single phrase, with no newline or trailing
punctuation."

So I think these extra messages (with complex formatting & content) are
better printed with error_printf() as I suggested in [1].

[1]: https://lore.kernel.org/qemu-devel/ZrWP0fWPNzeAvjja@intel.com/T/#m953afd879eb6279fcdf03cda594c43f1829bdffe

> +            error_report("- for kernels supporting the switch_amode or"
> +                        " user_mode parameters, whether");
> +            error_report("  user space is running in primary address space");
> +            error_report("- for kernels supporting the vm.allocate_pgste "
> +                        "sysctl, whether it is enabled");
>          }
>  #elif defined(TARGET_PPC)
>          if (ret == -EINVAL) {
> -            fprintf(stderr,
> -                    "PPC KVM module is not loaded. Try modprobe kvm_%s.\n",
> -                    (type == 2) ? "pr" : "hv");
> +            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
> +                        (type == 2) ? "pr" : "hv");

Same here. A trailing punctuation. If possible, feel free to refer to
the comment in [1].

>          }
>  #endif

[snip]

> @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms)
>      }
>      if (missing_cap) {
>          ret = -EINVAL;
> -        fprintf(stderr, "kvm does not support %s\n%s",
> -                missing_cap->name, upgrade_note);
> +        error_report("kvm does not support %s", missing_cap->name);
> +        error_report("%s", upgrade_note);

"upgrade_note" string also has the trailing punctuation, and it's
also better to use error_printf() to replace the 2nd error_report().

For this patch, error_report() is already a big step forward, so I think
these few nits doesn't block this patch.

Thank you for your patience.
Zhao



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions
  2024-08-09  6:13   ` Zhao Liu
@ 2024-08-09  7:06     ` Ani Sinha
  0 siblings, 0 replies; 8+ messages in thread
From: Ani Sinha @ 2024-08-09  7:06 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, qemu-trivial, kvm, qemu-devel

On Fri, Aug 9, 2024 at 11:27 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> On Fri, Aug 09, 2024 at 10:40:53AM +0530, Ani Sinha wrote:
> > Date: Fri,  9 Aug 2024 10:40:53 +0530
> > From: Ani Sinha <anisinha@redhat.com>
> > Subject: [PATCH v2 1/2] kvm: replace fprintf with error_report() in
> >  kvm_init() for error conditions
> > X-Mailer: git-send-email 2.45.2
> >
> > error_report() is more appropriate for error situations. Replace fprintf with
> > error_report. Cosmetic. No functional change.
> >
> > CC: qemu-trivial@nongnu.org
> > CC: zhao1.liu@intel.com
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> >  accel/kvm/kvm-all.c | 40 ++++++++++++++++++----------------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > changelog:
> > v2: fix a bug.
>
> Generally good to me. Only some nits below, otherwise,
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
> >  #ifdef TARGET_S390X
> >          if (ret == -EINVAL) {
> > -            fprintf(stderr,
> > -                    "Host kernel setup problem detected. Please verify:\n");
> > -            fprintf(stderr, "- for kernels supporting the switch_amode or"
> > -                    " user_mode parameters, whether\n");
> > -            fprintf(stderr,
> > -                    "  user space is running in primary address space\n");
> > -            fprintf(stderr,
> > -                    "- for kernels supporting the vm.allocate_pgste sysctl, "
> > -                    "whether it is enabled\n");
> > +            error_report("Host kernel setup problem detected. Please verify:");
>
> The doc of error_report() said it doesn't want multiple sentences or trailing
> punctuation:
>
> "The resulting message should be a single phrase, with no newline or trailing
> punctuation."
>
> So I think these extra messages (with complex formatting & content) are
> better printed with error_printf() as I suggested in [1].
>
> [1]: https://lore.kernel.org/qemu-devel/ZrWP0fWPNzeAvjja@intel.com/T/#m953afd879eb6279fcdf03cda594c43f1829bdffe
>
> > +            error_report("- for kernels supporting the switch_amode or"
> > +                        " user_mode parameters, whether");
> > +            error_report("  user space is running in primary address space");
> > +            error_report("- for kernels supporting the vm.allocate_pgste "
> > +                        "sysctl, whether it is enabled");
> >          }
> >  #elif defined(TARGET_PPC)
> >          if (ret == -EINVAL) {
> > -            fprintf(stderr,
> > -                    "PPC KVM module is not loaded. Try modprobe kvm_%s.\n",
> > -                    (type == 2) ? "pr" : "hv");
> > +            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
> > +                        (type == 2) ? "pr" : "hv");
>
> Same here. A trailing punctuation. If possible, feel free to refer to
> the comment in [1].

vreport() adds a training newline, so I think its ok to remove the
training newline and replace with error_report().

>
> >          }
> >  #endif
>
> [snip]
>
> > @@ -2542,8 +2538,8 @@ static int kvm_init(MachineState *ms)
> >      }
> >      if (missing_cap) {
> >          ret = -EINVAL;
> > -        fprintf(stderr, "kvm does not support %s\n%s",
> > -                missing_cap->name, upgrade_note);
> > +        error_report("kvm does not support %s", missing_cap->name);
> > +        error_report("%s", upgrade_note);
>
> "upgrade_note" string also has the trailing punctuation, and it's
> also better to use error_printf() to replace the 2nd error_report().

Yes this looks ugly and I will replace this one with error_printf()

>
> For this patch, error_report() is already a big step forward, so I think
> these few nits doesn't block this patch.

Thanks but I will send another version with your tag added.

>
> Thank you for your patience.
> Zhao
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function
  2024-08-09  5:10 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha
  2024-08-09  5:31   ` Zhao Liu
@ 2024-08-09  7:58   ` Claudio Fontana
  1 sibling, 0 replies; 8+ messages in thread
From: Claudio Fontana @ 2024-08-09  7:58 UTC (permalink / raw)
  To: Ani Sinha, Paolo Bonzini; +Cc: zhao1.liu, qemu-trivial, kvm, qemu-devel

gltm, thanks!

Reviewed-by: Claudio Fontana <cfontana@suse.de>

On 8/9/24 07:10, Ani Sinha wrote:
> Refactoring the core logic around KVM_CREATE_VM into its own separate function
> so that it can be called from other functions in subsequent patches. There is
> no functional change in this patch.
> 
> CC: pbonzini@redhat.com
> CC: zhao1.liu@intel.com
> CC: cfontana@suse.de
> CC: qemu-trivial@nongnu.org
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 86 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 53 insertions(+), 33 deletions(-)
> 
> changelog:
> v2: s/fprintf/warn_report as suggested by zhao
> v3: s/warn_report/error_report. function names adjusted to conform to
> other names. fprintf -> error_report() moved to its own patch.
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ac168b9663..610b3ead32 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2385,6 +2385,57 @@ uint32_t kvm_dirty_ring_size(void)
>      return kvm_state->kvm_dirty_ring_size;
>  }
>  
> +static int kvm_create_vm(MachineState *ms, KVMState *s, int type)
> +{
> +    int ret;
> +
> +    do {
> +        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
> +    } while (ret == -EINTR);
> +
> +    if (ret < 0) {
> +        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
> +                    strerror(-ret));
> +
> +#ifdef TARGET_S390X
> +        if (ret == -EINVAL) {
> +            error_report("Host kernel setup problem detected. Please verify:");
> +            error_report("- for kernels supporting the switch_amode or"
> +                        " user_mode parameters, whether");
> +            error_report("  user space is running in primary address space");
> +            error_report("- for kernels supporting the vm.allocate_pgste "
> +                        "sysctl, whether it is enabled");
> +        }
> +#elif defined(TARGET_PPC)
> +        if (ret == -EINVAL) {
> +            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
> +                        (type == 2) ? "pr" : "hv");
> +        }
> +#endif
> +    }
> +
> +    return ret;
> +}
> +
> +static int kvm_machine_type(MachineState *ms)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    int type;
> +
> +    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
> +        g_autofree char *kvm_type;
> +        kvm_type = object_property_get_str(OBJECT(current_machine),
> +                                           "kvm-type",
> +                                           &error_abort);
> +        type = mc->kvm_type(ms, kvm_type);
> +    } else if (mc->kvm_type) {
> +        type = mc->kvm_type(ms, NULL);
> +    } else {
> +        type = kvm_arch_get_default_type(ms);
> +    }
> +    return type;
> +}
> +
>  static int kvm_init(MachineState *ms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2467,45 +2518,14 @@ static int kvm_init(MachineState *ms)
>      }
>      s->as = g_new0(struct KVMAs, s->nr_as);
>  
> -    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
> -        g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine),
> -                                                            "kvm-type",
> -                                                            &error_abort);
> -        type = mc->kvm_type(ms, kvm_type);
> -    } else if (mc->kvm_type) {
> -        type = mc->kvm_type(ms, NULL);
> -    } else {
> -        type = kvm_arch_get_default_type(ms);
> -    }
> -
> +    type = kvm_machine_type(ms);
>      if (type < 0) {
>          ret = -EINVAL;
>          goto err;
>      }
>  
> -    do {
> -        ret = kvm_ioctl(s, KVM_CREATE_VM, type);
> -    } while (ret == -EINTR);
> -
> +    ret = kvm_create_vm(ms, s, type);
>      if (ret < 0) {
> -        error_report("ioctl(KVM_CREATE_VM) failed: %d %s", -ret,
> -                    strerror(-ret));
> -
> -#ifdef TARGET_S390X
> -        if (ret == -EINVAL) {
> -            error_report("Host kernel setup problem detected. Please verify:");
> -            error_report("- for kernels supporting the switch_amode or"
> -                        " user_mode parameters, whether");
> -            error_report("  user space is running in primary address space");
> -            error_report("- for kernels supporting the vm.allocate_pgste "
> -                        "sysctl, whether it is enabled");
> -        }
> -#elif defined(TARGET_PPC)
> -        if (ret == -EINVAL) {
> -            error_report("PPC KVM module is not loaded. Try modprobe kvm_%s.",
> -                        (type == 2) ? "pr" : "hv");
> -        }
> -#endif
>          goto err;
>      }
>  



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-08-09  7:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-09  5:10 [PATCH 0/2] Some refactoring Ani Sinha
2024-08-09  5:10 ` [PATCH v2 1/2] kvm: replace fprintf with error_report() in kvm_init() for error conditions Ani Sinha
2024-08-09  6:13   ` Zhao Liu
2024-08-09  7:06     ` Ani Sinha
2024-08-09  5:10 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha
2024-08-09  5:31   ` Zhao Liu
2024-08-09  7:58   ` Claudio Fontana
  -- strict thread matches above, loose matches on Subject: below --
2024-08-09  4:51 [PATCH 0/2] Some refactoring Ani Sinha
2024-08-09  4:51 ` [PATCH v3 2/2] kvm: refactor core virtual machine creation into its own function Ani Sinha

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.