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

[PATCH v3 0/4] 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.

v3: - Add preliminary patch to use warn_report() instead of
      a convoluted error_setg()+warn_report_err() sequence
v2: - Fix indentation and add some missing \n in patch 2
    - Add ERRP_AUTO_PROPAGATE() to cap_nested_kvm_hv_apply() in
      patch 2 instead of patch 3

---

Greg Kurz (3):
      spapr: Simplify some warning printing paths in spapr_caps.c
      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  |  127 ++++++++++++++++---------------
 include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 239 insertions(+), 93 deletions(-)

--
Greg

[PATCH v3 1/4] spapr: Simplify some warning printing paths in spapr_caps.c

[Greg Kurz]

We obviously only want to print a warning in these cases, but this is done
in a rather convoluted manner. Just use warn_report() instead.

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

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index efdc0dbbcfc0..0c2bc8e06e44 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -248,23 +248,18 @@ SpaprCapPossible cap_cfpc_possible = {
 static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
-    Error *local_err = NULL;
     uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
 
     if (tcg_enabled() && val) {
         /* TCG only supports broken, allow other values and print a warning */
-        error_setg(&local_err,
-                   "TCG doesn't support requested feature, cap-cfpc=%s",
-                   cap_cfpc_possible.vals[val]);
+        warn_report("TCG doesn't support requested feature, cap-cfpc=%s",
+                    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]);
     }
-
-    if (local_err != NULL)
-        warn_report_err(local_err);
 }
 
 SpaprCapPossible cap_sbbc_possible = {
@@ -277,23 +272,18 @@ SpaprCapPossible cap_sbbc_possible = {
 static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                                         Error **errp)
 {
-    Error *local_err = NULL;
     uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
 
     if (tcg_enabled() && val) {
         /* TCG only supports broken, allow other values and print a warning */
-        error_setg(&local_err,
-                   "TCG doesn't support requested feature, cap-sbbc=%s",
-                   cap_sbbc_possible.vals[val]);
+        warn_report("TCG doesn't support requested feature, cap-sbbc=%s",
+                    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]);
     }
-
-    if (local_err != NULL)
-        warn_report_err(local_err);
 }
 
 SpaprCapPossible cap_ibs_possible = {
@@ -309,24 +299,18 @@ SpaprCapPossible cap_ibs_possible = {
 static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
-    Error *local_err = NULL;
     uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
 
     if (tcg_enabled() && val) {
         /* TCG only supports broken, allow other values and print a warning */
-        error_setg(&local_err,
-                   "TCG doesn't support requested feature, cap-ibs=%s",
-                   cap_ibs_possible.vals[val]);
+        warn_report("TCG doesn't support requested feature, cap-ibs=%s",
+                    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]);
     }
-
-    if (local_err != NULL) {
-        warn_report_err(local_err);
-    }
 }
 
 #define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"

Re: [PATCH v3 1/4] spapr: Simplify some warning printing paths in spapr_caps.c

[Vladimir Sementsov-Ogievskiy]

11.06.2020 16:40, Greg Kurz wrote:
> We obviously only want to print a warning in these cases, but this is done
> in a rather convoluted manner. Just use warn_report() instead.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

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

-- 
Best regards,
Vladimir

Re: [PATCH v3 1/4] spapr: Simplify some warning printing paths in spapr_caps.c

[Laurent Vivier]

On 11/06/2020 15:40, Greg Kurz wrote:
> We obviously only want to print a warning in these cases, but this is done
> in a rather convoluted manner. Just use warn_report() instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_caps.c |   28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index efdc0dbbcfc0..0c2bc8e06e44 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -248,23 +248,18 @@ SpaprCapPossible cap_cfpc_possible = {
>  static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>                                   Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-cfpc=%s",
> -                   cap_cfpc_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-cfpc=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL)
> -        warn_report_err(local_err);
>  }
>  
>  SpaprCapPossible cap_sbbc_possible = {
> @@ -277,23 +272,18 @@ SpaprCapPossible cap_sbbc_possible = {
>  static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                                          Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-sbbc=%s",
> -                   cap_sbbc_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-sbbc=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL)
> -        warn_report_err(local_err);
>  }
>  
>  SpaprCapPossible cap_ibs_possible = {
> @@ -309,24 +299,18 @@ SpaprCapPossible cap_ibs_possible = {
>  static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                                             uint8_t val, Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-ibs=%s",
> -                   cap_ibs_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-ibs=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL) {
> -        warn_report_err(local_err);
> -    }
>  }
>  
>  #define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
> 
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH v3 1/4] spapr: Simplify some warning printing paths in spapr_caps.c

[David Gibson]

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

On Thu, Jun 11, 2020 at 03:40:11PM +0200, Greg Kurz wrote:
> We obviously only want to print a warning in these cases, but this is done
> in a rather convoluted manner. Just use warn_report() instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.1, thanks.

> ---
>  hw/ppc/spapr_caps.c |   28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index efdc0dbbcfc0..0c2bc8e06e44 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -248,23 +248,18 @@ SpaprCapPossible cap_cfpc_possible = {
>  static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>                                   Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-cfpc=%s",
> -                   cap_cfpc_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-cfpc=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL)
> -        warn_report_err(local_err);
>  }
>  
>  SpaprCapPossible cap_sbbc_possible = {
> @@ -277,23 +272,18 @@ SpaprCapPossible cap_sbbc_possible = {
>  static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                                          Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-sbbc=%s",
> -                   cap_sbbc_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-sbbc=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL)
> -        warn_report_err(local_err);
>  }
>  
>  SpaprCapPossible cap_ibs_possible = {
> @@ -309,24 +299,18 @@ SpaprCapPossible cap_ibs_possible = {
>  static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                                             uint8_t val, Error **errp)
>  {
> -    Error *local_err = NULL;
>      uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
>  
>      if (tcg_enabled() && val) {
>          /* TCG only supports broken, allow other values and print a warning */
> -        error_setg(&local_err,
> -                   "TCG doesn't support requested feature, cap-ibs=%s",
> -                   cap_ibs_possible.vals[val]);
> +        warn_report("TCG doesn't support requested feature, cap-ibs=%s",
> +                    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]);
>      }
> -
> -    if (local_err != NULL) {
> -        warn_report_err(local_err);
> -    }
>  }
>  
>  #define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
> 
> 

-- 
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 --]

[PATCH v3 2/4] 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>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
---
 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.

Re: [PATCH v3 2/4] error: auto propagated local_err

[David Gibson]

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

On Thu, Jun 11, 2020 at 03:40:18PM +0200, Greg Kurz wrote:
> 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>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

I don't feel terribly qualified to comment on this generic change to
the errors mechanism.  I can take it through my tree if necessary, but
I'd want an ack from Markus.

> ---
>  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.
> 
> 

-- 
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 v3 2/4] error: auto propagated local_err

[Greg Kurz]

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

On Sat, 13 Jun 2020 17:12:59 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 11, 2020 at 03:40:18PM +0200, Greg Kurz wrote:
> > 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>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> I don't feel terribly qualified to comment on this generic change to
> the errors mechanism.  I can take it through my tree if necessary, but
> I'd want an ack from Markus.
> 

Markus answered in another mail that there's still some more work to do
before we start using the ERRP_AUTO_PROPAGATE macro. A suivre...

> > ---
> >  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.
> > 
> > 
> 


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

[PATCH v3 3/4] 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, 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/ppc/spapr_caps.c |   89 +++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 0c2bc8e06e44..27cf2b38af27 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();
     uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
 
     if (tcg_enabled() && val) {
@@ -256,9 +258,9 @@ 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]);
     }
 }
 
@@ -272,6 +274,7 @@ SpaprCapPossible cap_sbbc_possible = {
 static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                                         Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
 
     if (tcg_enabled() && val) {
@@ -280,9 +283,9 @@ 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]);
     }
 }
 
@@ -299,6 +302,7 @@ SpaprCapPossible cap_ibs_possible = {
 static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
 
     if (tcg_enabled() && val) {
@@ -307,9 +311,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]);
     }
 }
 
@@ -386,23 +390,25 @@ 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();
     if (!val) {
         /* capability disabled by default */
         return;
     }
 
     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\n");
     } 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\n");
         } 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\n");
         }
     }
 }
@@ -410,6 +416,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);
 
@@ -420,22 +427,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");
         }
     }
 }
@@ -444,14 +452,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");
         }
     }
 
@@ -466,6 +475,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) {
@@ -488,14 +498,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 v3 3/4] spapr: Use error_append_hint() in spapr_caps.c

[Laurent Vivier]

On 11/06/2020 15:40, 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, 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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/ppc/spapr_caps.c |   89 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 0c2bc8e06e44..27cf2b38af27 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();
>      uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>  
>      if (tcg_enabled() && val) {
> @@ -256,9 +258,9 @@ 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]);
>      }
>  }
>  
> @@ -272,6 +274,7 @@ SpaprCapPossible cap_sbbc_possible = {
>  static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                                          Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();
>      uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
>  
>      if (tcg_enabled() && val) {
> @@ -280,9 +283,9 @@ 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]);
>      }
>  }
>  
> @@ -299,6 +302,7 @@ SpaprCapPossible cap_ibs_possible = {
>  static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                                             uint8_t val, Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();
>      uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
>  
>      if (tcg_enabled() && val) {
> @@ -307,9 +311,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]);
>      }
>  }
>  
> @@ -386,23 +390,25 @@ 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();
>      if (!val) {
>          /* capability disabled by default */
>          return;
>      }
>  
>      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\n");
>      } 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\n");
>          } 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\n");
>          }
>      }
>  }
> @@ -410,6 +416,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);
>  
> @@ -420,22 +427,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");
>          }
>      }
>  }
> @@ -444,14 +452,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");
>          }
>      }
>  
> @@ -466,6 +475,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) {
> @@ -488,14 +498,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 */
>      }
> 
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[PATCH v3 4/4] 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>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/ppc/spapr_caps.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 27cf2b38af27..dfe3b419daaa 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -391,6 +391,8 @@ 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;
@@ -400,6 +402,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\n");
     } 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 v3 4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[David Gibson]

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

On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> Nested KVM-HV only works on POWER9.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Hrm.  I have mixed feelings about this.  It does bring forward an
error that we'd otherwise only discover when we try to load the kvm
module in the guest.

On the other hand, it's kind of a layering violation - really it's
KVM's business to report what it can and can't do, rather than having
qemu anticipate it.

Allowing POWER8 compat for an L2 is something we hope to have in the
fairly near future.  Allowing POWER8 compat for L1, which is what this
covers, is, I'll admit, likely to never happen.


> ---
>  HW/ppc/spapr_caps.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 27cf2b38af27..dfe3b419daaa 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -391,6 +391,8 @@ 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;
> @@ -400,6 +402,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\n");
>      } 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");
> 
> 

-- 
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 v3 4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

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

On Sat, 13 Jun 2020 17:18:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > Nested KVM-HV only works on POWER9.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> Hrm.  I have mixed feelings about this.  It does bring forward an
> error that we'd otherwise only discover when we try to load the kvm
> module in the guest.
> 
> On the other hand, it's kind of a layering violation - really it's
> KVM's business to report what it can and can't do, rather than having
> qemu anticipate it.
> 

Agreed and it seems that we can probably get KVM to report that
already. I'll have closer look.

> Allowing POWER8 compat for an L2 is something we hope to have in the
> fairly near future.

Ok but I guess we don't want to start an L2 in compat POWER8 mode
with cap-nested-hv=on, do we ?

>  Allowing POWER8 compat for L1, which is what this
> covers, is, I'll admit, likely to never happen.
> 
> 
> > ---
> >  HW/ppc/spapr_caps.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 27cf2b38af27..dfe3b419daaa 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -391,6 +391,8 @@ 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;
> > @@ -400,6 +402,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\n");
> >      } 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");
> > 
> > 
> 


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

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

[David Gibson]

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

On Mon, Jun 15, 2020 at 11:20:31AM +0200, Greg Kurz wrote:
> On Sat, 13 Jun 2020 17:18:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > Nested KVM-HV only works on POWER9.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Hrm.  I have mixed feelings about this.  It does bring forward an
> > error that we'd otherwise only discover when we try to load the kvm
> > module in the guest.
> > 
> > On the other hand, it's kind of a layering violation - really it's
> > KVM's business to report what it can and can't do, rather than having
> > qemu anticipate it.
> > 
> 
> Agreed and it seems that we can probably get KVM to report that
> already. I'll have closer look.
> 
> > Allowing POWER8 compat for an L2 is something we hope to have in the
> > fairly near future.
> 
> Ok but I guess we don't want to start an L2 in compat POWER8 mode
> with cap-nested-hv=on, do we ?

Sorry, "L2" was misleading, I really mean L<max>.  Setting
cap-nested-hv kind of implies there's a L<at least one more> which
contradicts that.

-- 
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 v3 4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

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

On Mon, 15 Jun 2020 11:20:31 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Sat, 13 Jun 2020 17:18:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > Nested KVM-HV only works on POWER9.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Hrm.  I have mixed feelings about this.  It does bring forward an
> > error that we'd otherwise only discover when we try to load the kvm
> > module in the guest.
> > 
> > On the other hand, it's kind of a layering violation - really it's
> > KVM's business to report what it can and can't do, rather than having
> > qemu anticipate it.
> > 
> 
> Agreed and it seems that we can probably get KVM to report that
> already. I'll have closer look.
> 

Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
supports. It can't reasonably take into account that we're going to
switch vCPUs in some compat mode later on. KVM could possibly check
that it has a vCPU in pre-power9 compat mode when we try to enable
the capability and fail... but it would be a layering violation all
the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
the host, it is the one in the guest, and it's not even directly
related to the CPU type but to the MMU mode currently in use:

long kvmhv_nested_init(void)
{
	long int ptb_order;
	unsigned long ptcr;
	long rc;

	if (!kvmhv_on_pseries())
		return 0;
==>	if (!radix_enabled())
		return -ENODEV;

We cannot know either for sure the MMU mode the guest will run in
when we enable the nested cap during the initial machine reset.
So it seems we cannot do anything better than denylisting well
known broken setups, in which case QEMU seems a better fit than
KVM.

Makes sense ?

> > Allowing POWER8 compat for an L2 is something we hope to have in the
> > fairly near future.
> 
> Ok but I guess we don't want to start an L2 in compat POWER8 mode
> with cap-nested-hv=on, do we ?
> 
> >  Allowing POWER8 compat for L1, which is what this
> > covers, is, I'll admit, likely to never happen.
> > 
> > 
> > > ---
> > >  HW/ppc/spapr_caps.c |   10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > index 27cf2b38af27..dfe3b419daaa 100644
> > > --- a/hw/ppc/spapr_caps.c
> > > +++ b/hw/ppc/spapr_caps.c
> > > @@ -391,6 +391,8 @@ 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;
> > > @@ -400,6 +402,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\n");
> > >      } 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");
> > > 
> > > 
> > 
> 


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

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

[David Gibson]

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

On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> On Mon, 15 Jun 2020 11:20:31 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Sat, 13 Jun 2020 17:18:04 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > Nested KVM-HV only works on POWER9.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > > 
> > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > error that we'd otherwise only discover when we try to load the kvm
> > > module in the guest.
> > > 
> > > On the other hand, it's kind of a layering violation - really it's
> > > KVM's business to report what it can and can't do, rather than having
> > > qemu anticipate it.
> > > 
> > 
> > Agreed and it seems that we can probably get KVM to report that
> > already. I'll have closer look.
> > 
> 
> Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> supports. It can't reasonably take into account that we're going to
> switch vCPUs in some compat mode later on. KVM could possibly check
> that it has a vCPU in pre-power9 compat mode when we try to enable
> the capability and fail... but it would be a layering violation all
> the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> the host, it is the one in the guest, and it's not even directly
> related to the CPU type but to the MMU mode currently in use:
> 
> long kvmhv_nested_init(void)
> {
> 	long int ptb_order;
> 	unsigned long ptcr;
> 	long rc;
> 
> 	if (!kvmhv_on_pseries())
> 		return 0;
> ==>	if (!radix_enabled())
> 		return -ENODEV;
> 
> We cannot know either for sure the MMU mode the guest will run in
> when we enable the nested cap during the initial machine reset.
> So it seems we cannot do anything better than denylisting well
> known broken setups, in which case QEMU seems a better fit than
> KVM.
> 
> Makes sense ?

Yeah, good points.

-- 
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 v3 4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

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

On Mon, 13 Jul 2020 14:53:30 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> > On Mon, 15 Jun 2020 11:20:31 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Sat, 13 Jun 2020 17:18:04 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > > Nested KVM-HV only works on POWER9.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > > > 
> > > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > > error that we'd otherwise only discover when we try to load the kvm
> > > > module in the guest.
> > > > 
> > > > On the other hand, it's kind of a layering violation - really it's
> > > > KVM's business to report what it can and can't do, rather than having
> > > > qemu anticipate it.
> > > > 
> > > 
> > > Agreed and it seems that we can probably get KVM to report that
> > > already. I'll have closer look.
> > > 
> > 
> > Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> > supports. It can't reasonably take into account that we're going to
> > switch vCPUs in some compat mode later on. KVM could possibly check
> > that it has a vCPU in pre-power9 compat mode when we try to enable
> > the capability and fail... but it would be a layering violation all
> > the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> > the host, it is the one in the guest, and it's not even directly
> > related to the CPU type but to the MMU mode currently in use:
> > 
> > long kvmhv_nested_init(void)
> > {
> > 	long int ptb_order;
> > 	unsigned long ptcr;
> > 	long rc;
> > 
> > 	if (!kvmhv_on_pseries())
> > 		return 0;
> > ==>	if (!radix_enabled())
> > 		return -ENODEV;
> > 
> > We cannot know either for sure the MMU mode the guest will run in
> > when we enable the nested cap during the initial machine reset.
> > So it seems we cannot do anything better than denylisting well
> > known broken setups, in which case QEMU seems a better fit than
> > KVM.
> > 
> > Makes sense ?
> 
> Yeah, good points.
> 

So, should I just rebase/repost this or do you think of another
way ?

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

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

[David Gibson]

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

On Wed, Jul 15, 2020 at 01:14:42PM +0200, Greg Kurz wrote:
> On Mon, 13 Jul 2020 14:53:30 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 03, 2020 at 04:19:24PM +0200, Greg Kurz wrote:
> > > On Mon, 15 Jun 2020 11:20:31 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > > 
> > > > On Sat, 13 Jun 2020 17:18:04 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > > > > Nested KVM-HV only works on POWER9.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > > > > 
> > > > > Hrm.  I have mixed feelings about this.  It does bring forward an
> > > > > error that we'd otherwise only discover when we try to load the kvm
> > > > > module in the guest.
> > > > > 
> > > > > On the other hand, it's kind of a layering violation - really it's
> > > > > KVM's business to report what it can and can't do, rather than having
> > > > > qemu anticipate it.
> > > > > 
> > > > 
> > > > Agreed and it seems that we can probably get KVM to report that
> > > > already. I'll have closer look.
> > > > 
> > > 
> > > Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
> > > supports. It can't reasonably take into account that we're going to
> > > switch vCPUs in some compat mode later on. KVM could possibly check
> > > that it has a vCPU in pre-power9 compat mode when we try to enable
> > > the capability and fail... but it would be a layering violation all
> > > the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
> > > the host, it is the one in the guest, and it's not even directly
> > > related to the CPU type but to the MMU mode currently in use:
> > > 
> > > long kvmhv_nested_init(void)
> > > {
> > > 	long int ptb_order;
> > > 	unsigned long ptcr;
> > > 	long rc;
> > > 
> > > 	if (!kvmhv_on_pseries())
> > > 		return 0;
> > > ==>	if (!radix_enabled())
> > > 		return -ENODEV;
> > > 
> > > We cannot know either for sure the MMU mode the guest will run in
> > > when we enable the nested cap during the initial machine reset.
> > > So it seems we cannot do anything better than denylisting well
> > > known broken setups, in which case QEMU seems a better fit than
> > > KVM.
> > > 
> > > Makes sense ?
> > 
> > Yeah, good points.
> > 
> 
> So, should I just rebase/repost this or do you think of another
> way ?

Urgh... I've kind of forgotten the context while I've been away.  So,
I guess repost and I'll take another look at them.

-- 
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 --]