[PATCH 0/3] spapr: Improve error reporting in spapr_caps.c

[PATCH 0/3] spapr: Improve error reporting in spapr_caps.c

[Greg Kurz]

Spapr capabilities are checked at machine init. If a capability cannot
be used, an error message is printed and QEMU exits. In most places,
the error message also contains an hint for the user. But we should
use error_append_hint() for that, as explained in the "qapi/error.h"
header.

This is already the case for cap_fwnmi_apply() and we now want to add 
a similar check for nested KVM-HV. Unfortunately, spapr_caps_apply()
passes &error_fatal to all apply hooks and error_append_hint() is
never called. 

So this reuses previous work from Vladimir Sementsov-Ogievskiy to
address that.

---

Greg Kurz (2):
      spapr: Use error_append_hint() in spapr_caps.c
      spapr: Forbid nested KVM-HV in pre-power9 compat mode

Vladimir Sementsov-Ogievskiy (1):
      error: auto propagated local_err


 hw/ppc/spapr_caps.c  |  104 +++++++++++++++----------
 include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 236 insertions(+), 73 deletions(-)

--
Greg

[PATCH 1/3] error: auto propagated local_err

[Greg Kurz]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 173 insertions(+), 32 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index ad5b6e896ded..30140d9bfea9 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -15,6 +15,8 @@
 /*
  * Error reporting system loosely patterned after Glib's GError.
  *
+ * = Deal with Error object =
+ *
  * Create an error:
  *     error_setg(&err, "situation normal, all fouled up");
  *
@@ -47,28 +49,91 @@
  * reporting it (primarily useful in testsuites):
  *     error_free_or_abort(&err);
  *
- * Pass an existing error to the caller:
- *     error_propagate(errp, err);
- * where Error **errp is a parameter, by convention the last one.
+ * = Deal with Error ** function parameter =
  *
- * Pass an existing error to the caller with the message modified:
- *     error_propagate_prepend(errp, err);
+ * A function may use the error system to return errors. In this case, the
+ * function defines an Error **errp parameter, by convention the last one (with
+ * exceptions for functions using ... or va_list).
  *
- * Avoid
- *     error_propagate(errp, err);
- *     error_prepend(errp, "Could not frobnicate '%s': ", name);
- * because this fails to prepend when @errp is &error_fatal.
+ * The caller may then pass in the following errp values:
  *
- * Create a new error and pass it to the caller:
+ * 1. &error_abort
+ *    Any error will result in abort().
+ * 2. &error_fatal
+ *    Any error will result in exit() with a non-zero status.
+ * 3. NULL
+ *    No error reporting through errp parameter.
+ * 4. The address of a NULL-initialized Error *err
+ *    Any error will populate errp with an error object.
+ *
+ * The following rules then implement the correct semantics desired by the
+ * caller.
+ *
+ * Create a new error to pass to the caller:
  *     error_setg(errp, "situation normal, all fouled up");
  *
- * Call a function and receive an error from it:
+ * Calling another errp-based function:
+ *     f(..., errp);
+ *
+ * == Checking success of subcall ==
+ *
+ * If a function returns a value indicating an error in addition to setting
+ * errp (which is recommended), then you don't need any additional code, just
+ * do:
+ *
+ *     int ret = f(..., errp);
+ *     if (ret < 0) {
+ *         ... handle error ...
+ *         return ret;
+ *     }
+ *
+ * If a function returns nothing (not recommended for new code), the only way
+ * to check success is by consulting errp; doing this safely requires the use
+ * of the ERRP_AUTO_PROPAGATE macro, like this:
+ *
+ *     int our_func(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         ...
+ *         subcall(..., errp);
+ *         if (*errp) {
+ *             ...
+ *             return -EINVAL;
+ *         }
+ *         ...
+ *     }
+ *
+ * ERRP_AUTO_PROPAGATE takes care of wrapping the original errp as needed, so
+ * that the rest of the function can directly use errp (including
+ * dereferencing), where any errors will then be propagated on to the original
+ * errp when leaving the function.
+ *
+ * In some cases, we need to check result of subcall, but do not want to
+ * propagate the Error object to our caller. In such cases we don't need
+ * ERRP_AUTO_PROPAGATE, but just a local Error object:
+ *
+ * Receive an error and not pass it:
  *     Error *err = NULL;
- *     foo(arg, &err);
+ *     subcall(arg, &err);
  *     if (err) {
  *         handle the error...
+ *         error_free(err);
  *     }
  *
+ * Note that older code that did not use ERRP_AUTO_PROPAGATE would instead need
+ * a local Error * variable and the use of error_propagate() to properly handle
+ * all possible caller values of errp. Now this is DEPRECATED* (see below).
+ *
+ * Note that any function that wants to modify an error object, such as by
+ * calling error_append_hint or error_prepend, must use ERRP_AUTO_PROPAGATE, in
+ * order for a caller's use of &error_fatal to see the additional information.
+ *
+ * In rare cases, we need to pass existing Error object to the caller by hand:
+ *     error_propagate(errp, err);
+ *
+ * Pass an existing error to the caller with the message modified:
+ *     error_propagate_prepend(errp, err);
+ *
+ *
  * Call a function ignoring errors:
  *     foo(arg, NULL);
  *
@@ -78,26 +143,6 @@
  * Call a function treating errors as fatal:
  *     foo(arg, &error_fatal);
  *
- * Receive an error and pass it on to the caller:
- *     Error *err = NULL;
- *     foo(arg, &err);
- *     if (err) {
- *         handle the error...
- *         error_propagate(errp, err);
- *     }
- * where Error **errp is a parameter, by convention the last one.
- *
- * Do *not* "optimize" this to
- *     foo(arg, errp);
- *     if (*errp) { // WRONG!
- *         handle the error...
- *     }
- * because errp may be NULL!
- *
- * But when all you do with the error is pass it on, please use
- *     foo(arg, errp);
- * for readability.
- *
  * Receive and accumulate multiple errors (first one wins):
  *     Error *err = NULL, *local_err = NULL;
  *     foo(arg, &err);
@@ -114,6 +159,61 @@
  *         handle the error...
  *     }
  * because this may pass a non-null err to bar().
+ *
+ * DEPRECATED*
+ *
+ * The following pattern of receiving, checking, and then forwarding an error
+ * to the caller by hand is now deprecated:
+ *
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *         error_propagate(errp, err);
+ *     }
+ *
+ * Instead, use ERRP_AUTO_PROPAGATE macro.
+ *
+ * The old pattern is deprecated because of two things:
+ *
+ * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
+ * local_err+error_propagate, the resulting coredump will refer to
+ * error_propagate and not to the place where error happened.
+ *
+ * 2. A lot of extra code of the same pattern
+ *
+ * How to update old code to use ERRP_AUTO_PROPAGATE?
+ *
+ * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
+ * than you may safely dereference errp to check errors and do not need any
+ * additional local Error variables or calls to error_propagate().
+ *
+ * Example:
+ *
+ * old code
+ *
+ *     void fn(..., Error **errp) {
+ *         Error *err = NULL;
+ *         foo(arg, &err);
+ *         if (err) {
+ *             handle the error...
+ *             error_propagate(errp, err);
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ * updated code
+ *
+ *     void fn(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         foo(arg, errp);
+ *         if (*errp) {
+ *             handle the error...
+ *             return;
+ *         }
+ *         ...
+ *     }
  */
 
 #ifndef ERROR_H
@@ -322,6 +422,47 @@ void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro exists to assist with proper error handling in a function which
+ * uses an Error **errp parameter.  It must be used as the first line of a
+ * function which modifies an error (with error_prepend, error_append_hint, or
+ * similar) or which wants to dereference *errp.  It is still safe (but
+ * useless) to use in other functions.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information by error_prepend or
+ * error_append_hint (as, if it was error_fatal, we swapped it with a
+ * local_error to be propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+    do { \
+        if (!errp || errp == &error_fatal) { \
+            errp = &_auto_errp_prop.local_err; \
+        } \
+    } while (0)
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.

[PATCH 2/3] spapr: Use error_append_hint() in spapr_caps.c

[Greg Kurz]

We have a dedicated error API for hints. Use it instead of embedding
the hint in the error message, as recommanded in the "qapi/error.h"
header file.

Since spapr_caps_apply() passes &error_fatal, all functions must
also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint()
to be functional.

While here, add some missing braces around one line statements that
are part of the patch context. Also have cap_fwnmi_apply(), which
already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as
well.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_caps.c |   93 +++++++++++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index efdc0dbbcfc0..0c3d3b64a508 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -189,24 +189,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name,
 
 static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (!val) {
         /* TODO: We don't support disabling htm yet */
         return;
     }
     if (tcg_enabled()) {
-        error_setg(errp,
-                   "No Transactional Memory support in TCG,"
-                   " try appending -machine cap-htm=off");
+        error_setg(errp, "No Transactional Memory support in TCG");
+        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
     } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
         error_setg(errp,
-"KVM implementation does not support Transactional Memory,"
-                   " try appending -machine cap-htm=off"
-            );
+"KVM implementation does not support Transactional Memory");
+        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
     }
 }
 
 static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -218,13 +218,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
      * rid of anything that doesn't do VMX */
     g_assert(env->insns_flags & PPC_ALTIVEC);
     if (!(env->insns_flags2 & PPC2_VSX)) {
-        error_setg(errp, "VSX support not available,"
-                   " try appending -machine cap-vsx=off");
+        error_setg(errp, "VSX support not available");
+        error_append_hint(errp, "Try appending -machine cap-vsx=off\n");
     }
 }
 
 static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -233,8 +234,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
         return;
     }
     if (!(env->insns_flags2 & PPC2_DFP)) {
-        error_setg(errp, "DFP support not available,"
-                   " try appending -machine cap-dfp=off");
+        error_setg(errp, "DFP support not available");
+        error_append_hint(errp, "Try appending -machine cap-dfp=off\n");
     }
 }
 
@@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = {
 static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
     uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
 
@@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
                    cap_cfpc_possible.vals[val]);
     } else if (kvm_enabled() && (val > kvm_val)) {
         error_setg(errp,
-                   "Requested safe cache capability level not supported by kvm,"
-                   " try appending -machine cap-cfpc=%s",
-                   cap_cfpc_possible.vals[kvm_val]);
+"Requested safe cache capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n",
+                          cap_cfpc_possible.vals[kvm_val]);
     }
 
-    if (local_err != NULL)
+    if (local_err != NULL) {
         warn_report_err(local_err);
+    }
 }
 
 SpaprCapPossible cap_sbbc_possible = {
@@ -277,6 +280,7 @@ SpaprCapPossible cap_sbbc_possible = {
 static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                                         Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
     uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
 
@@ -287,13 +291,14 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                    cap_sbbc_possible.vals[val]);
     } else if (kvm_enabled() && (val > kvm_val)) {
         error_setg(errp,
-"Requested safe bounds check capability level not supported by kvm,"
-                   " try appending -machine cap-sbbc=%s",
-                   cap_sbbc_possible.vals[kvm_val]);
+"Requested safe bounds check capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n",
+                          cap_sbbc_possible.vals[kvm_val]);
     }
 
-    if (local_err != NULL)
+    if (local_err != NULL) {
         warn_report_err(local_err);
+    }
 }
 
 SpaprCapPossible cap_ibs_possible = {
@@ -309,6 +314,7 @@ SpaprCapPossible cap_ibs_possible = {
 static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
     uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
 
@@ -319,9 +325,9 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                    cap_ibs_possible.vals[val]);
     } else if (kvm_enabled() && (val > kvm_val)) {
         error_setg(errp,
-"Requested safe indirect branch capability level not supported by kvm,"
-                   " try appending -machine cap-ibs=%s",
-                   cap_ibs_possible.vals[kvm_val]);
+"Requested safe indirect branch capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-ibs=%s\n",
+                          cap_ibs_possible.vals[kvm_val]);
     }
 
     if (local_err != NULL) {
@@ -408,17 +414,17 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
     }
 
     if (tcg_enabled()) {
-        error_setg(errp,
-                   "No Nested KVM-HV support in tcg,"
-                   " try appending -machine cap-nested-hv=off");
+        error_setg(errp, "No Nested KVM-HV support in TCG");
+        error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
     } else if (kvm_enabled()) {
         if (!kvmppc_has_cap_nested_kvm_hv()) {
             error_setg(errp,
-"KVM implementation does not support Nested KVM-HV,"
-                       " try appending -machine cap-nested-hv=off");
+"KVM implementation does not support Nested KVM-HV");
+            error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
         } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
-                error_setg(errp,
-"Error enabling cap-nested-hv with KVM, try cap-nested-hv=off");
+                error_setg(errp, "Error enabling cap-nested-hv with KVM");
+                error_append_hint(errp,
+                                  "Try appending -machine cap-nested-hv=off");
         }
     }
 }
@@ -426,6 +432,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
 static void cap_large_decr_apply(SpaprMachineState *spapr,
                                  uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
@@ -436,22 +443,23 @@ static void cap_large_decr_apply(SpaprMachineState *spapr,
     if (tcg_enabled()) {
         if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
                               spapr->max_compat_pvr)) {
-            error_setg(errp,
-                "Large decrementer only supported on POWER9, try -cpu POWER9");
+            error_setg(errp, "Large decrementer only supported on POWER9");
+            error_append_hint(errp, "Try -cpu POWER9\n");
             return;
         }
     } else if (kvm_enabled()) {
         int kvm_nr_bits = kvmppc_get_cap_large_decr();
 
         if (!kvm_nr_bits) {
-            error_setg(errp,
-                       "No large decrementer support,"
-                        " try appending -machine cap-large-decr=off");
+            error_setg(errp, "No large decrementer support");
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         } else if (pcc->lrg_decr_bits != kvm_nr_bits) {
             error_setg(errp,
-"KVM large decrementer size (%d) differs to model (%d),"
-                " try appending -machine cap-large-decr=off",
-                kvm_nr_bits, pcc->lrg_decr_bits);
+                       "KVM large decrementer size (%d) differs to model (%d)",
+                       kvm_nr_bits, pcc->lrg_decr_bits);
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         }
     }
 }
@@ -460,14 +468,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
                                      PowerPCCPU *cpu,
                                      uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     CPUPPCState *env = &cpu->env;
     target_ulong lpcr = env->spr[SPR_LPCR];
 
     if (kvm_enabled()) {
         if (kvmppc_enable_cap_large_decr(cpu, val)) {
-            error_setg(errp,
-                       "No large decrementer support,"
-                       " try appending -machine cap-large-decr=off");
+            error_setg(errp, "No large decrementer support");
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         }
     }
 
@@ -482,6 +491,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
 static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
 
     if (tcg_enabled() && val) {
@@ -504,14 +514,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
             return;
         }
         error_setg(errp,
-"Requested count cache flush assist capability level not supported by kvm,"
-                   " try appending -machine cap-ccf-assist=off");
+"Requested count cache flush assist capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-ccf-assist=off\n");
     }
 }
 
 static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
                                 Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (!val) {
         return; /* Disabled by default */
     }

Re: [PATCH 2/3] spapr: Use error_append_hint() in spapr_caps.c

[Vladimir Sementsov-Ogievskiy]

10.06.2020 20:17, Greg Kurz wrote:
> We have a dedicated error API for hints. Use it instead of embedding
> the hint in the error message, as recommanded in the "qapi/error.h"
> header file.
> 
> Since spapr_caps_apply() passes &error_fatal, all functions must
> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint()
> to be functional.
> 
> While here, add some missing braces around one line statements that
> are part of the patch context. Also have cap_fwnmi_apply(), which
> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as
> well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/spapr_caps.c |   93 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 52 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index efdc0dbbcfc0..0c3d3b64a508 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -189,24 +189,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name,
>   
>   static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       if (!val) {
>           /* TODO: We don't support disabling htm yet */
>           return;
>       }
>       if (tcg_enabled()) {
> -        error_setg(errp,
> -                   "No Transactional Memory support in TCG,"
> -                   " try appending -machine cap-htm=off");
> +        error_setg(errp, "No Transactional Memory support in TCG");
> +        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
>       } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
>           error_setg(errp,
> -"KVM implementation does not support Transactional Memory,"
> -                   " try appending -machine cap-htm=off"
> -            );
> +"KVM implementation does not support Transactional Memory");

Should be indented after opening '('

> +        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
>       }
>   }
>   
>   static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>       CPUPPCState *env = &cpu->env;
>   
> @@ -218,13 +218,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>        * rid of anything that doesn't do VMX */
>       g_assert(env->insns_flags & PPC_ALTIVEC);
>       if (!(env->insns_flags2 & PPC2_VSX)) {
> -        error_setg(errp, "VSX support not available,"
> -                   " try appending -machine cap-vsx=off");
> +        error_setg(errp, "VSX support not available");
> +        error_append_hint(errp, "Try appending -machine cap-vsx=off\n");
>       }
>   }
>   
>   static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>       CPUPPCState *env = &cpu->env;
>   
> @@ -233,8 +234,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
>           return;
>       }
>       if (!(env->insns_flags2 & PPC2_DFP)) {
> -        error_setg(errp, "DFP support not available,"
> -                   " try appending -machine cap-dfp=off");
> +        error_setg(errp, "DFP support not available");
> +        error_append_hint(errp, "Try appending -machine cap-dfp=off\n");
>       }
>   }
>   
> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = {
>   static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>                                    Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       Error *local_err = NULL;
>       uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>   
> @@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>                      cap_cfpc_possible.vals[val]);
>       } else if (kvm_enabled() && (val > kvm_val)) {
>           error_setg(errp,
> -                   "Requested safe cache capability level not supported by kvm,"
> -                   " try appending -machine cap-cfpc=%s",
> -                   cap_cfpc_possible.vals[kvm_val]);
> +"Requested safe cache capability level not supported by KVM");

Hmm you do this intentionally.. OK, than, it's a kind of taste.

> +        error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n",
> +                          cap_cfpc_possible.vals[kvm_val]);
>       }
>   
> -    if (local_err != NULL)
> +    if (local_err != NULL) {
>           warn_report_err(local_err);
> +    }
>   }
>   
>   SpaprCapPossible cap_sbbc_possible = {
> @@ -277,6 +280,7 @@ SpaprCapPossible cap_sbbc_possible = {
>   static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                                           Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       Error *local_err = NULL;
>       uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
>   
> @@ -287,13 +291,14 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                      cap_sbbc_possible.vals[val]);
>       } else if (kvm_enabled() && (val > kvm_val)) {
>           error_setg(errp,
> -"Requested safe bounds check capability level not supported by kvm,"
> -                   " try appending -machine cap-sbbc=%s",
> -                   cap_sbbc_possible.vals[kvm_val]);
> +"Requested safe bounds check capability level not supported by KVM");
> +        error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n",
> +                          cap_sbbc_possible.vals[kvm_val]);
>       }
>   
> -    if (local_err != NULL)
> +    if (local_err != NULL) {
>           warn_report_err(local_err);
> +    }
>   }
>   
>   SpaprCapPossible cap_ibs_possible = {
> @@ -309,6 +314,7 @@ SpaprCapPossible cap_ibs_possible = {
>   static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                                              uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       Error *local_err = NULL;
>       uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
>   
> @@ -319,9 +325,9 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                      cap_ibs_possible.vals[val]);
>       } else if (kvm_enabled() && (val > kvm_val)) {
>           error_setg(errp,
> -"Requested safe indirect branch capability level not supported by kvm,"
> -                   " try appending -machine cap-ibs=%s",
> -                   cap_ibs_possible.vals[kvm_val]);
> +"Requested safe indirect branch capability level not supported by KVM");
> +        error_append_hint(errp, "Try appending -machine cap-ibs=%s\n",
> +                          cap_ibs_possible.vals[kvm_val]);
>       }
>   
>       if (local_err != NULL) {
> @@ -408,17 +414,17 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,

You forget to add ERRP_AUTO_PROPAGATE

>       }
>   
>       if (tcg_enabled()) {
> -        error_setg(errp,
> -                   "No Nested KVM-HV support in tcg,"
> -                   " try appending -machine cap-nested-hv=off");
> +        error_setg(errp, "No Nested KVM-HV support in TCG");
> +        error_append_hint(errp, "Try appending -machine cap-nested-hv=off");

Hmm, didn't you forget '\n' ? You consistantly add it in previous hints. (I do think that it's strange that we should add it by hand, but it seems a common thing to add it)

>       } else if (kvm_enabled()) {
>           if (!kvmppc_has_cap_nested_kvm_hv()) {
>               error_setg(errp,
> -"KVM implementation does not support Nested KVM-HV,"
> -                       " try appending -machine cap-nested-hv=off");
> +"KVM implementation does not support Nested KVM-HV");
> +            error_append_hint(errp, "Try appending -machine cap-nested-hv=off");

and here

>           } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
> -                error_setg(errp,
> -"Error enabling cap-nested-hv with KVM, try cap-nested-hv=off");
> +                error_setg(errp, "Error enabling cap-nested-hv with KVM");
> +                error_append_hint(errp,
> +                                  "Try appending -machine cap-nested-hv=off");

and here

>           }
>       }
>   }
> @@ -426,6 +432,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>   static void cap_large_decr_apply(SpaprMachineState *spapr,
>                                    uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>   
> @@ -436,22 +443,23 @@ static void cap_large_decr_apply(SpaprMachineState *spapr,
>       if (tcg_enabled()) {
>           if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
>                                 spapr->max_compat_pvr)) {
> -            error_setg(errp,
> -                "Large decrementer only supported on POWER9, try -cpu POWER9");
> +            error_setg(errp, "Large decrementer only supported on POWER9");
> +            error_append_hint(errp, "Try -cpu POWER9\n");
>               return;
>           }
>       } else if (kvm_enabled()) {
>           int kvm_nr_bits = kvmppc_get_cap_large_decr();
>   
>           if (!kvm_nr_bits) {
> -            error_setg(errp,
> -                       "No large decrementer support,"
> -                        " try appending -machine cap-large-decr=off");
> +            error_setg(errp, "No large decrementer support");
> +            error_append_hint(errp,
> +                              "Try appending -machine cap-large-decr=off\n");
>           } else if (pcc->lrg_decr_bits != kvm_nr_bits) {
>               error_setg(errp,
> -"KVM large decrementer size (%d) differs to model (%d),"
> -                " try appending -machine cap-large-decr=off",
> -                kvm_nr_bits, pcc->lrg_decr_bits);
> +                       "KVM large decrementer size (%d) differs to model (%d)",
> +                       kvm_nr_bits, pcc->lrg_decr_bits);
> +            error_append_hint(errp,
> +                              "Try appending -machine cap-large-decr=off\n");
>           }
>       }
>   }
> @@ -460,14 +468,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
>                                        PowerPCCPU *cpu,
>                                        uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       CPUPPCState *env = &cpu->env;
>       target_ulong lpcr = env->spr[SPR_LPCR];
>   
>       if (kvm_enabled()) {
>           if (kvmppc_enable_cap_large_decr(cpu, val)) {
> -            error_setg(errp,
> -                       "No large decrementer support,"
> -                       " try appending -machine cap-large-decr=off");
> +            error_setg(errp, "No large decrementer support");
> +            error_append_hint(errp,
> +                              "Try appending -machine cap-large-decr=off\n");
>           }
>       }
>   
> @@ -482,6 +491,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
>   static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
>                                    Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
>   
>       if (tcg_enabled() && val) {
> @@ -504,14 +514,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
>               return;
>           }
>           error_setg(errp,
> -"Requested count cache flush assist capability level not supported by kvm,"
> -                   " try appending -machine cap-ccf-assist=off");
> +"Requested count cache flush assist capability level not supported by KVM");
> +        error_append_hint(errp, "Try appending -machine cap-ccf-assist=off\n");
>       }
>   }
>   
>   static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>                                   Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       if (!val) {
>           return; /* Disabled by default */
>       }
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/3] spapr: Use error_append_hint() in spapr_caps.c

[Greg Kurz]

On Wed, 10 Jun 2020 21:03:15 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 10.06.2020 20:17, Greg Kurz wrote:
> > We have a dedicated error API for hints. Use it instead of embedding
> > the hint in the error message, as recommanded in the "qapi/error.h"
> > header file.
> > 
> > Since spapr_caps_apply() passes &error_fatal, all functions must
> > also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint()
> > to be functional.
> > 
> > While here, add some missing braces around one line statements that
> > are part of the patch context. Also have cap_fwnmi_apply(), which
> > already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as
> > well.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   hw/ppc/spapr_caps.c |   93 +++++++++++++++++++++++++++++----------------------
> >   1 file changed, 52 insertions(+), 41 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index efdc0dbbcfc0..0c3d3b64a508 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -189,24 +189,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name,
> >   
> >   static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       if (!val) {
> >           /* TODO: We don't support disabling htm yet */
> >           return;
> >       }
> >       if (tcg_enabled()) {
> > -        error_setg(errp,
> > -                   "No Transactional Memory support in TCG,"
> > -                   " try appending -machine cap-htm=off");
> > +        error_setg(errp, "No Transactional Memory support in TCG");
> > +        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
> >       } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> >           error_setg(errp,
> > -"KVM implementation does not support Transactional Memory,"
> > -                   " try appending -machine cap-htm=off"
> > -            );
> > +"KVM implementation does not support Transactional Memory");
> 
> Should be indented after opening '('
> 

Many error_setg() lines in this file follow the same style of tweaking
indentation to stay below the 80 character limit. I've just kept the
existing style.

> > +        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
> >       }
> >   }
> >   
> >   static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >       CPUPPCState *env = &cpu->env;
> >   
> > @@ -218,13 +218,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >        * rid of anything that doesn't do VMX */
> >       g_assert(env->insns_flags & PPC_ALTIVEC);
> >       if (!(env->insns_flags2 & PPC2_VSX)) {
> > -        error_setg(errp, "VSX support not available,"
> > -                   " try appending -machine cap-vsx=off");
> > +        error_setg(errp, "VSX support not available");
> > +        error_append_hint(errp, "Try appending -machine cap-vsx=off\n");
> >       }
> >   }
> >   
> >   static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >       CPUPPCState *env = &cpu->env;
> >   
> > @@ -233,8 +234,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
> >           return;
> >       }
> >       if (!(env->insns_flags2 & PPC2_DFP)) {
> > -        error_setg(errp, "DFP support not available,"
> > -                   " try appending -machine cap-dfp=off");
> > +        error_setg(errp, "DFP support not available");
> > +        error_append_hint(errp, "Try appending -machine cap-dfp=off\n");
> >       }
> >   }
> >   
> > @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = {
> >   static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
> >                                    Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       Error *local_err = NULL;
> >       uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
> >   
> > @@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
> >                      cap_cfpc_possible.vals[val]);
> >       } else if (kvm_enabled() && (val > kvm_val)) {
> >           error_setg(errp,
> > -                   "Requested safe cache capability level not supported by kvm,"
> > -                   " try appending -machine cap-cfpc=%s",
> > -                   cap_cfpc_possible.vals[kvm_val]);
> > +"Requested safe cache capability level not supported by KVM");
> 
> Hmm you do this intentionally.. OK, than, it's a kind of taste.
> 

Yes, but as said above, there's precedence in this file.

I'm okay with fixing the indent. It is acceptable to have error_setg()
lines span over 80 characters for the sake of being able to grep error
strings in the code base.

> > +        error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n",
> > +                          cap_cfpc_possible.vals[kvm_val]);
> >       }
> >   
> > -    if (local_err != NULL)
> > +    if (local_err != NULL) {
> >           warn_report_err(local_err);
> > +    }
> >   }
> >   
> >   SpaprCapPossible cap_sbbc_possible = {
> > @@ -277,6 +280,7 @@ SpaprCapPossible cap_sbbc_possible = {
> >   static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
> >                                           Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       Error *local_err = NULL;
> >       uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
> >   
> > @@ -287,13 +291,14 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
> >                      cap_sbbc_possible.vals[val]);
> >       } else if (kvm_enabled() && (val > kvm_val)) {
> >           error_setg(errp,
> > -"Requested safe bounds check capability level not supported by kvm,"
> > -                   " try appending -machine cap-sbbc=%s",
> > -                   cap_sbbc_possible.vals[kvm_val]);
> > +"Requested safe bounds check capability level not supported by KVM");
> > +        error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n",
> > +                          cap_sbbc_possible.vals[kvm_val]);
> >       }
> >   
> > -    if (local_err != NULL)
> > +    if (local_err != NULL) {
> >           warn_report_err(local_err);
> > +    }
> >   }
> >   
> >   SpaprCapPossible cap_ibs_possible = {
> > @@ -309,6 +314,7 @@ SpaprCapPossible cap_ibs_possible = {
> >   static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
> >                                              uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       Error *local_err = NULL;
> >       uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
> >   
> > @@ -319,9 +325,9 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
> >                      cap_ibs_possible.vals[val]);
> >       } else if (kvm_enabled() && (val > kvm_val)) {
> >           error_setg(errp,
> > -"Requested safe indirect branch capability level not supported by kvm,"
> > -                   " try appending -machine cap-ibs=%s",
> > -                   cap_ibs_possible.vals[kvm_val]);
> > +"Requested safe indirect branch capability level not supported by KVM");
> > +        error_append_hint(errp, "Try appending -machine cap-ibs=%s\n",
> > +                          cap_ibs_possible.vals[kvm_val]);
> >       }
> >   
> >       if (local_err != NULL) {
> > @@ -408,17 +414,17 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> 
> You forget to add ERRP_AUTO_PROPAGATE
> 

Oops, it's in the next patch... I've re-ordered the patches and
I simply forgot to move that hunk here. I will fix that in v2.

> >       }
> >   
> >       if (tcg_enabled()) {
> > -        error_setg(errp,
> > -                   "No Nested KVM-HV support in tcg,"
> > -                   " try appending -machine cap-nested-hv=off");
> > +        error_setg(errp, "No Nested KVM-HV support in TCG");
> > +        error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
> 
> Hmm, didn't you forget '\n' ? You consistantly add it in previous hints. (I do think that it's strange that we should add it by hand, but it seems a common thing to add it)
> 

Oops indeed I forgot. Good catch ! :)

From the documentation of error_append_hint() in "qapi/error.h" :

 * May be called multiple times.  The resulting hint should end with a
 * newline.

> >       } else if (kvm_enabled()) {
> >           if (!kvmppc_has_cap_nested_kvm_hv()) {
> >               error_setg(errp,
> > -"KVM implementation does not support Nested KVM-HV,"
> > -                       " try appending -machine cap-nested-hv=off");
> > +"KVM implementation does not support Nested KVM-HV");
> > +            error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
> 
> and here
> 

Oops

> >           } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
> > -                error_setg(errp,
> > -"Error enabling cap-nested-hv with KVM, try cap-nested-hv=off");
> > +                error_setg(errp, "Error enabling cap-nested-hv with KVM");
> > +                error_append_hint(errp,
> > +                                  "Try appending -machine cap-nested-hv=off");
> 
> and here
> 

Thanks for all the catches !

> >           }
> >       }
> >   }
> > @@ -426,6 +432,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> >   static void cap_large_decr_apply(SpaprMachineState *spapr,
> >                                    uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >   
> > @@ -436,22 +443,23 @@ static void cap_large_decr_apply(SpaprMachineState *spapr,
> >       if (tcg_enabled()) {
> >           if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> >                                 spapr->max_compat_pvr)) {
> > -            error_setg(errp,
> > -                "Large decrementer only supported on POWER9, try -cpu POWER9");
> > +            error_setg(errp, "Large decrementer only supported on POWER9");
> > +            error_append_hint(errp, "Try -cpu POWER9\n");
> >               return;
> >           }
> >       } else if (kvm_enabled()) {
> >           int kvm_nr_bits = kvmppc_get_cap_large_decr();
> >   
> >           if (!kvm_nr_bits) {
> > -            error_setg(errp,
> > -                       "No large decrementer support,"
> > -                        " try appending -machine cap-large-decr=off");
> > +            error_setg(errp, "No large decrementer support");
> > +            error_append_hint(errp,
> > +                              "Try appending -machine cap-large-decr=off\n");
> >           } else if (pcc->lrg_decr_bits != kvm_nr_bits) {
> >               error_setg(errp,
> > -"KVM large decrementer size (%d) differs to model (%d),"
> > -                " try appending -machine cap-large-decr=off",
> > -                kvm_nr_bits, pcc->lrg_decr_bits);
> > +                       "KVM large decrementer size (%d) differs to model (%d)",
> > +                       kvm_nr_bits, pcc->lrg_decr_bits);
> > +            error_append_hint(errp,
> > +                              "Try appending -machine cap-large-decr=off\n");
> >           }
> >       }
> >   }
> > @@ -460,14 +468,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> >                                        PowerPCCPU *cpu,
> >                                        uint8_t val, Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       CPUPPCState *env = &cpu->env;
> >       target_ulong lpcr = env->spr[SPR_LPCR];
> >   
> >       if (kvm_enabled()) {
> >           if (kvmppc_enable_cap_large_decr(cpu, val)) {
> > -            error_setg(errp,
> > -                       "No large decrementer support,"
> > -                       " try appending -machine cap-large-decr=off");
> > +            error_setg(errp, "No large decrementer support");
> > +            error_append_hint(errp,
> > +                              "Try appending -machine cap-large-decr=off\n");
> >           }
> >       }
> >   
> > @@ -482,6 +491,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
> >   static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> >                                    Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
> >   
> >       if (tcg_enabled() && val) {
> > @@ -504,14 +514,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> >               return;
> >           }
> >           error_setg(errp,
> > -"Requested count cache flush assist capability level not supported by kvm,"
> > -                   " try appending -machine cap-ccf-assist=off");
> > +"Requested count cache flush assist capability level not supported by KVM");
> > +        error_append_hint(errp, "Try appending -machine cap-ccf-assist=off\n");
> >       }
> >   }
> >   
> >   static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
> >                                   Error **errp)
> >   {
> > +    ERRP_AUTO_PROPAGATE();
> >       if (!val) {
> >           return; /* Disabled by default */
> >       }
> > 
> > 
> 
> 

[PATCH 3/3] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

Nested KVM-HV only works on POWER9.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_caps.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 0c3d3b64a508..05c8f70506ad 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -408,6 +408,9 @@ static void cap_hpt_maxpagesize_cpu_apply(SpaprMachineState *spapr,
 static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                                     uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+
     if (!val) {
         /* capability disabled by default */
         return;
@@ -417,6 +420,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
         error_setg(errp, "No Nested KVM-HV support in TCG");
         error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
     } else if (kvm_enabled()) {
+        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
+                              spapr->max_compat_pvr)) {
+            error_setg(errp, "Nested KVM-HV only supported on POWER9");
+            error_append_hint(errp,
+                              "Try appending -machine max-cpu-compat=power9\n");
+            return;
+        }
+
         if (!kvmppc_has_cap_nested_kvm_hv()) {
             error_setg(errp,
 "KVM implementation does not support Nested KVM-HV");

Re: [PATCH 3/3] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Vladimir Sementsov-Ogievskiy]

10.06.2020 20:17, Greg Kurz wrote:
> Nested KVM-HV only works on POWER9.

Worth mention that existing usage of error_append_hint in the function is fixed too by this patch?

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/spapr_caps.c |   11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 0c3d3b64a508..05c8f70506ad 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -408,6 +408,9 @@ static void cap_hpt_maxpagesize_cpu_apply(SpaprMachineState *spapr,
>   static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>                                       uint8_t val, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +
>       if (!val) {
>           /* capability disabled by default */
>           return;
> @@ -417,6 +420,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>           error_setg(errp, "No Nested KVM-HV support in TCG");
>           error_append_hint(errp, "Try appending -machine cap-nested-hv=off");
>       } else if (kvm_enabled()) {
> +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> +                              spapr->max_compat_pvr)) {
> +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> +            error_append_hint(errp,
> +                              "Try appending -machine max-cpu-compat=power9\n");

I think you should either add \n to all other hint in this function or drop this \n.

> +            return;
> +        }
> +
>           if (!kvmppc_has_cap_nested_kvm_hv()) {
>               error_setg(errp,
>   "KVM implementation does not support Nested KVM-HV");
> 
> 


-- 
Best regards,
Vladimir