[PATCH v4 for-5.2 0/2] spapr: Improve error reporting in spapr_caps.c

[PATCH v4 for-5.2 0/2] spapr: Improve error reporting in spapr_caps.c

[Greg Kurz]

Nested KVM HV only works if the kernel is using the radix MMU mode, ie.
the CPU is POWER9 and it is not running in some pre-power9 compat mode.
Otherwise, the KVM HV module fails to load in the guest with -ENODEV.
It might be painful for a user to discover this late that nested cannot
work with their setup. It seems a better fit for QEMU to do a sanity
check when applying the nested-hv sPAPR capability and print out an
informative error message.

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.

So this series first converts spapr_caps.c to using error_append_hint().
This requires to add some ERRP_GUARD() because spapr_caps_apply() passes
&error_fatal. Then it adds a sanity check for the nested-hv case with
an error message and hint.

v4: - Same as v3 but rebased on ppc-for-5.2, updated changelogs
      and cover
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 (2):
      spapr: Use error_append_hint() in spapr_caps.c
      spapr: Forbid nested KVM-HV in pre-power9 compat mode


 hw/ppc/spapr_caps.c |   99 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 39 deletions(-)

--
Greg

[PATCH v4 for-5.2 1/2] 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_GUARD() macro for error_append_hint() to be
functional.

While here, have cap_fwnmi_apply(), which already uses
error_append_hint(), to call ERRP_GUARD() as well.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.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 3225fc5a2edc..275f5bd0342c 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -180,24 +180,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_GUARD();
     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_GUARD();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -209,13 +209,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_GUARD();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -224,8 +225,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");
     }
 }
 
@@ -239,6 +240,7 @@ SpaprCapPossible cap_cfpc_possible = {
 static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
+    ERRP_GUARD();
     uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
 
     if (tcg_enabled() && val) {
@@ -247,9 +249,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]);
     }
 }
 
@@ -263,6 +265,7 @@ SpaprCapPossible cap_sbbc_possible = {
 static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                                         Error **errp)
 {
+    ERRP_GUARD();
     uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
 
     if (tcg_enabled() && val) {
@@ -271,9 +274,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]);
     }
 }
 
@@ -290,6 +293,7 @@ SpaprCapPossible cap_ibs_possible = {
 static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
+    ERRP_GUARD();
     uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
 
     if (tcg_enabled() && val) {
@@ -298,9 +302,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]);
     }
 }
 
@@ -377,23 +381,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_GUARD();
     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");
         }
     }
 }
@@ -401,6 +407,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
 static void cap_large_decr_apply(SpaprMachineState *spapr,
                                  uint8_t val, Error **errp)
 {
+    ERRP_GUARD();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
@@ -411,22 +418,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");
         }
     }
 }
@@ -435,14 +443,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
                                      PowerPCCPU *cpu,
                                      uint8_t val, Error **errp)
 {
+    ERRP_GUARD();
     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");
         }
     }
 
@@ -457,6 +466,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
 static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
+    ERRP_GUARD();
     uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
 
     if (tcg_enabled() && val) {
@@ -479,14 +489,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_GUARD();
     if (!val) {
         return; /* Disabled by default */
     }

Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[David Gibson]

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

On Thu, Jul 16, 2020 at 07:11:11PM +0200, 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_GUARD() macro for error_append_hint() to be
> functional.
> 
> While here, have cap_fwnmi_apply(), which already uses
> error_append_hint(), to call ERRP_GUARD() as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Applied to ppc-for-5.2.

> ---
>  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 3225fc5a2edc..275f5bd0342c 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -180,24 +180,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_GUARD();
>      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_GUARD();
>      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>      CPUPPCState *env = &cpu->env;
>  
> @@ -209,13 +209,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_GUARD();
>      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>      CPUPPCState *env = &cpu->env;
>  
> @@ -224,8 +225,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");
>      }
>  }
>  
> @@ -239,6 +240,7 @@ SpaprCapPossible cap_cfpc_possible = {
>  static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>                                   Error **errp)
>  {
> +    ERRP_GUARD();
>      uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>  
>      if (tcg_enabled() && val) {
> @@ -247,9 +249,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]);
>      }
>  }
>  
> @@ -263,6 +265,7 @@ SpaprCapPossible cap_sbbc_possible = {
>  static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
>                                          Error **errp)
>  {
> +    ERRP_GUARD();
>      uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
>  
>      if (tcg_enabled() && val) {
> @@ -271,9 +274,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]);
>      }
>  }
>  
> @@ -290,6 +293,7 @@ SpaprCapPossible cap_ibs_possible = {
>  static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
>                                             uint8_t val, Error **errp)
>  {
> +    ERRP_GUARD();
>      uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
>  
>      if (tcg_enabled() && val) {
> @@ -298,9 +302,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]);
>      }
>  }
>  
> @@ -377,23 +381,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_GUARD();
>      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");
>          }
>      }
>  }
> @@ -401,6 +407,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>  static void cap_large_decr_apply(SpaprMachineState *spapr,
>                                   uint8_t val, Error **errp)
>  {
> +    ERRP_GUARD();
>      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
> @@ -411,22 +418,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");
>          }
>      }
>  }
> @@ -435,14 +443,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
>                                       PowerPCCPU *cpu,
>                                       uint8_t val, Error **errp)
>  {
> +    ERRP_GUARD();
>      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");
>          }
>      }
>  
> @@ -457,6 +466,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
>  static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
>                                   Error **errp)
>  {
> +    ERRP_GUARD();
>      uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
>  
>      if (tcg_enabled() && val) {
> @@ -479,14 +489,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_GUARD();
>      if (!val) {
>          return; /* Disabled by default */
>      }
> 
> 

-- 
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 v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> 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_GUARD() macro for error_append_hint() to be
> functional.

This isn't a request for change in this patch, just an attempt to squash
possible misunderstandings.

It's true that error_append_hint() without ERRP_GUARD() works as long as
the caller doesn't pass certain errp arguments.  But the callee should
work for all possible @errp arguments, not just the ones that get passed
today.  That's why error.h wants you to guard *all* uses of
error_append_hint(errp):

 * = Why, when and how to use ERRP_GUARD() =
 *
 * Without ERRP_GUARD(), use of the @errp parameter is restricted:
 * - It must not be dereferenced, because it may be null.
 * - It should not be passed to error_prepend() or
 *   error_append_hint(), because that doesn't work with &error_fatal.
 * ERRP_GUARD() lifts these restrictions.

No need to make an argument involving the possible arguments (pardon the
pun).

[...]

Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[Greg Kurz]

On Mon, 20 Jul 2020 17:24:35 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > 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_GUARD() macro for error_append_hint() to be
> > functional.
> 
> This isn't a request for change in this patch, just an attempt to squash
> possible misunderstandings.
> 
> It's true that error_append_hint() without ERRP_GUARD() works as long as
> the caller doesn't pass certain errp arguments.  But the callee should
> work for all possible @errp arguments, not just the ones that get passed
> today.  That's why error.h wants you to guard *all* uses of
> error_append_hint(errp):
> 
>  * = Why, when and how to use ERRP_GUARD() =
>  *
>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>  * - It must not be dereferenced, because it may be null.
>  * - It should not be passed to error_prepend() or
>  *   error_append_hint(), because that doesn't work with &error_fatal.
>  * ERRP_GUARD() lifts these restrictions.
> 

Yeah, I just wanted to emphasize that we were precisely in the case
where we _really_ need to lift the restriction, but I'm perfectly fine
with dropping this sentence if you consider it useless.

BTW, should we have a way for CI to ensure that a patch that adds
error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
ERRP_GUARD() ? Not sure that people read error.h that often...

> No need to make an argument involving the possible arguments (pardon the
> pun).
> 

:)

> [...]
> 

Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Mon, 20 Jul 2020 17:24:35 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Greg Kurz <groug@kaod.org> writes:
>> 
>> > 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_GUARD() macro for error_append_hint() to be
>> > functional.
>> 
>> This isn't a request for change in this patch, just an attempt to squash
>> possible misunderstandings.
>> 
>> It's true that error_append_hint() without ERRP_GUARD() works as long as
>> the caller doesn't pass certain errp arguments.  But the callee should
>> work for all possible @errp arguments, not just the ones that get passed
>> today.  That's why error.h wants you to guard *all* uses of
>> error_append_hint(errp):
>> 
>>  * = Why, when and how to use ERRP_GUARD() =
>>  *
>>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>>  * - It must not be dereferenced, because it may be null.
>>  * - It should not be passed to error_prepend() or
>>  *   error_append_hint(), because that doesn't work with &error_fatal.
>>  * ERRP_GUARD() lifts these restrictions.
>> 
>
> Yeah, I just wanted to emphasize that we were precisely in the case
> where we _really_ need to lift the restriction, but I'm perfectly fine
> with dropping this sentence if you consider it useless.

I lean towards dropping it.

> BTW, should we have a way for CI to ensure that a patch that adds
> error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
> ERRP_GUARD() ? Not sure that people read error.h that often...

I don't know.  Wait and see whether it's worth automating?  We didn't
automate checking other Error API rules, like "no newlines in error
messages".  That one can't crash, though.

The check would have to look beyond the patch, which checkpatch.pl
doesn't do.

>> No need to make an argument involving the possible arguments (pardon the
>> pun).
>> 
>
> :)
>
>> [...]
>> 

Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[Greg Kurz]

On Tue, 28 Jul 2020 09:26:08 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Mon, 20 Jul 2020 17:24:35 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Greg Kurz <groug@kaod.org> writes:
> >> 
> >> > 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_GUARD() macro for error_append_hint() to be
> >> > functional.
> >> 
> >> This isn't a request for change in this patch, just an attempt to squash
> >> possible misunderstandings.
> >> 
> >> It's true that error_append_hint() without ERRP_GUARD() works as long as
> >> the caller doesn't pass certain errp arguments.  But the callee should
> >> work for all possible @errp arguments, not just the ones that get passed
> >> today.  That's why error.h wants you to guard *all* uses of
> >> error_append_hint(errp):
> >> 
> >>  * = Why, when and how to use ERRP_GUARD() =
> >>  *
> >>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> >>  * - It must not be dereferenced, because it may be null.
> >>  * - It should not be passed to error_prepend() or
> >>  *   error_append_hint(), because that doesn't work with &error_fatal.
> >>  * ERRP_GUARD() lifts these restrictions.
> >> 
> >
> > Yeah, I just wanted to emphasize that we were precisely in the case
> > where we _really_ need to lift the restriction, but I'm perfectly fine
> > with dropping this sentence if you consider it useless.
> 
> I lean towards dropping it.
> 

David,

Do you want me to send an updated version of this patch or can you
fix this in your tree ?

> > BTW, should we have a way for CI to ensure that a patch that adds
> > error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
> > ERRP_GUARD() ? Not sure that people read error.h that often...
> 
> I don't know.  Wait and see whether it's worth automating?  We didn't
> automate checking other Error API rules, like "no newlines in error
> messages".  That one can't crash, though.
> 
> The check would have to look beyond the patch, which checkpatch.pl
> doesn't do.
> 

<thinking aloud>
Maybe checkpatch.pl could be fed with an extended version of the patch
that has enough context, eg. git show -U$(wc -l ${file}) ${file} ?
</thinking aloud>

> >> No need to make an argument involving the possible arguments (pardon the
> >> pun).
> >> 
> >
> > :)
> >
> >> [...]
> >> 
> 

Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[David Gibson]

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

On Tue, Jul 28, 2020 at 11:07:26AM +0200, Greg Kurz wrote:
> On Tue, 28 Jul 2020 09:26:08 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Greg Kurz <groug@kaod.org> writes:
> > 
> > > On Mon, 20 Jul 2020 17:24:35 +0200
> > > Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > >> Greg Kurz <groug@kaod.org> writes:
> > >> 
> > >> > 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_GUARD() macro for error_append_hint() to be
> > >> > functional.
> > >> 
> > >> This isn't a request for change in this patch, just an attempt to squash
> > >> possible misunderstandings.
> > >> 
> > >> It's true that error_append_hint() without ERRP_GUARD() works as long as
> > >> the caller doesn't pass certain errp arguments.  But the callee should
> > >> work for all possible @errp arguments, not just the ones that get passed
> > >> today.  That's why error.h wants you to guard *all* uses of
> > >> error_append_hint(errp):
> > >> 
> > >>  * = Why, when and how to use ERRP_GUARD() =
> > >>  *
> > >>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > >>  * - It must not be dereferenced, because it may be null.
> > >>  * - It should not be passed to error_prepend() or
> > >>  *   error_append_hint(), because that doesn't work with &error_fatal.
> > >>  * ERRP_GUARD() lifts these restrictions.
> > >> 
> > >
> > > Yeah, I just wanted to emphasize that we were precisely in the case
> > > where we _really_ need to lift the restriction, but I'm perfectly fine
> > > with dropping this sentence if you consider it useless.
> > 
> > I lean towards dropping it.
> > 
> 
> David,
> 
> Do you want me to send an updated version of this patch or can you
> fix this in your tree ?

Updated version please, I'm afraid I've lost track of quite what was
suggested in this thread.

> 
> > > BTW, should we have a way for CI to ensure that a patch that adds
> > > error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
> > > ERRP_GUARD() ? Not sure that people read error.h that often...
> > 
> > I don't know.  Wait and see whether it's worth automating?  We didn't
> > automate checking other Error API rules, like "no newlines in error
> > messages".  That one can't crash, though.
> > 
> > The check would have to look beyond the patch, which checkpatch.pl
> > doesn't do.
> > 
> 
> <thinking aloud>
> Maybe checkpatch.pl could be fed with an extended version of the patch
> that has enough context, eg. git show -U$(wc -l ${file}) ${file} ?
> </thinking aloud>
> 
> > >> No need to make an argument involving the possible arguments (pardon the
> > >> pun).
> > >> 
> > >
> > > :)
> > >
> > >> [...]
> > >> 
> > 
> 

-- 
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 v4 for-5.2 2/2] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

Nested KVM HV only works if the kernel is using the radix MMU mode, ie.
the CPU is POWER9 and it is not running in some pre-power9 compat mode.
Otherwise, the KVM HV module fails to load in the guest with -ENODEV.
It might be painful for a user to discover this late that nested cannot
work with their setup. Erroring out at machine init instead seems to be
the best we can do.

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 275f5bd0342c..10a80a8159f4 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -382,6 +382,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                                     uint8_t val, Error **errp)
 {
     ERRP_GUARD();
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+
     if (!val) {
         /* capability disabled by default */
         return;
@@ -391,6 +393,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 v4 for-5.2 2/2] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[David Gibson]

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

On Thu, Jul 16, 2020 at 07:11:21PM +0200, Greg Kurz wrote:
> Nested KVM HV only works if the kernel is using the radix MMU mode, ie.
> the CPU is POWER9 and it is not running in some pre-power9 compat mode.
> Otherwise, the KVM HV module fails to load in the guest with -ENODEV.
> It might be painful for a user to discover this late that nested cannot
> work with their setup. Erroring out at machine init instead seems to be
> the best we can do.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Applied to ppc-for-5.2.

> ---
>  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 275f5bd0342c..10a80a8159f4 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -382,6 +382,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>                                      uint8_t val, Error **errp)
>  {
>      ERRP_GUARD();
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +
>      if (!val) {
>          /* capability disabled by default */
>          return;
> @@ -391,6 +393,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 --]