[PATCH v4 01/15] spapr: nested: register nested-hv api hcalls only for cap-nested-hv

Harsh Prateek Bora posted 15 patches 8 months, 3 weeks ago
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>
There is a newer version of this series
[PATCH v4 01/15] spapr: nested: register nested-hv api hcalls only for cap-nested-hv
Posted by Harsh Prateek Bora 8 months, 3 weeks ago
Since cap-nested-hv is an optional capability, it makes sense to register
api specfic hcalls only when respective capability is enabled. This
requires to introduce a new API to unregister hypercalls to maintain
sanity across guest reboot since caps are re-applied across reboots and
re-registeration of hypercalls would hit assert otherwise.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/ppc/spapr.h        |  3 +++
 include/hw/ppc/spapr_nested.h |  1 -
 hw/ppc/spapr_caps.c           |  2 ++
 hw/ppc/spapr_hcall.c          | 24 ++++++++++++++++++++++--
 hw/ppc/spapr_nested.c         | 17 +++++++++++++++--
 5 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index e91791a1a9..b7ad2a0360 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -631,6 +631,7 @@ typedef target_ulong (*spapr_hcall_fn)(PowerPCCPU *cpu, SpaprMachineState *sm,
                                        target_ulong *args);
 
 void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
+void spapr_unregister_hypercall(target_ulong opcode);
 target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
                              target_ulong *args);
 
@@ -1025,5 +1026,7 @@ void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
 
 /* H_WATCHDOG */
 void spapr_watchdog_init(SpaprMachineState *spapr);
+void spapr_register_nested_hv(void);
+void spapr_unregister_nested_hv(void);
 
 #endif /* HW_SPAPR_H */
diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
index d312a5d61d..09d95182b2 100644
--- a/include/hw/ppc/spapr_nested.h
+++ b/include/hw/ppc/spapr_nested.h
@@ -95,7 +95,6 @@ struct nested_ppc_state {
     int64_t tb_offset;
 };
 
-void spapr_register_nested(void);
 void spapr_exit_nested(PowerPCCPU *cpu, int excp);
 
 #endif /* HW_SPAPR_NESTED_H */
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index e889244e52..f0c2f4de78 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -487,6 +487,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
             error_append_hint(errp, "Try appending -machine cap-nested-hv=off "
                                     "or use threads=1 with -smp\n");
         }
+        spapr_unregister_nested_hv(); /* reset across reboots */
+        spapr_register_nested_hv();
     }
 }
 
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index fcefd1d1c7..8dbc2863a8 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1521,6 +1521,28 @@ void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn)
     *slot = fn;
 }
 
+void spapr_unregister_hypercall(target_ulong opcode)
+{
+    spapr_hcall_fn *slot;
+
+    if (opcode <= MAX_HCALL_OPCODE) {
+        assert((opcode & 0x3) == 0);
+
+        slot = &papr_hypercall_table[opcode / 4];
+    } else if (opcode >= SVM_HCALL_BASE && opcode <= SVM_HCALL_MAX) {
+        /* we only have SVM-related hcall numbers assigned in multiples of 4 */
+        assert((opcode & 0x3) == 0);
+
+        slot = &svm_hypercall_table[(opcode - SVM_HCALL_BASE) / 4];
+    } else {
+        assert((opcode >= KVMPPC_HCALL_BASE) && (opcode <= KVMPPC_HCALL_MAX));
+
+        slot = &kvmppc_hypercall_table[opcode - KVMPPC_HCALL_BASE];
+    }
+
+    *slot = NULL;
+}
+
 target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
                              target_ulong *args)
 {
@@ -1634,8 +1656,6 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
 
     spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
-
-    spapr_register_nested();
 }
 
 type_init(hypercall_register_types)
diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
index 121aa96ddc..a071092eff 100644
--- a/hw/ppc/spapr_nested.c
+++ b/hw/ppc/spapr_nested.c
@@ -375,20 +375,33 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     address_space_unmap(CPU(cpu)->as, regs, len, len, true);
 }
 
-void spapr_register_nested(void)
+void spapr_register_nested_hv(void)
 {
     spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
     spapr_register_hypercall(KVMPPC_H_ENTER_NESTED, h_enter_nested);
     spapr_register_hypercall(KVMPPC_H_TLB_INVALIDATE, h_tlb_invalidate);
     spapr_register_hypercall(KVMPPC_H_COPY_TOFROM_GUEST, h_copy_tofrom_guest);
 }
+
+void spapr_unregister_nested_hv(void)
+{
+    spapr_unregister_hypercall(KVMPPC_H_SET_PARTITION_TABLE);
+    spapr_unregister_hypercall(KVMPPC_H_ENTER_NESTED);
+    spapr_unregister_hypercall(KVMPPC_H_TLB_INVALIDATE);
+    spapr_unregister_hypercall(KVMPPC_H_COPY_TOFROM_GUEST);
+}
 #else
 void spapr_exit_nested(PowerPCCPU *cpu, int excp)
 {
     g_assert_not_reached();
 }
 
-void spapr_register_nested(void)
+void spapr_register_nested_hv(void)
+{
+    /* DO NOTHING */
+}
+
+void spapr_unregister_nested_hv(void)
 {
     /* DO NOTHING */
 }
-- 
2.39.3
Re: [PATCH v4 01/15] spapr: nested: register nested-hv api hcalls only for cap-nested-hv
Posted by Nicholas Piggin 8 months, 2 weeks ago
On Tue Feb 20, 2024 at 6:35 PM AEST, Harsh Prateek Bora wrote:
> Since cap-nested-hv is an optional capability, it makes sense to register
> api specfic hcalls only when respective capability is enabled. This
> requires to introduce a new API to unregister hypercalls to maintain
> sanity across guest reboot since caps are re-applied across reboots and
> re-registeration of hypercalls would hit assert otherwise.
>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  include/hw/ppc/spapr.h        |  3 +++
>  include/hw/ppc/spapr_nested.h |  1 -
>  hw/ppc/spapr_caps.c           |  2 ++
>  hw/ppc/spapr_hcall.c          | 24 ++++++++++++++++++++++--
>  hw/ppc/spapr_nested.c         | 17 +++++++++++++++--
>  5 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e91791a1a9..b7ad2a0360 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -631,6 +631,7 @@ typedef target_ulong (*spapr_hcall_fn)(PowerPCCPU *cpu, SpaprMachineState *sm,
>                                         target_ulong *args);
>  
>  void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
> +void spapr_unregister_hypercall(target_ulong opcode);
>  target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>                               target_ulong *args);
>  
> @@ -1025,5 +1026,7 @@ void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>  
>  /* H_WATCHDOG */
>  void spapr_watchdog_init(SpaprMachineState *spapr);
> +void spapr_register_nested_hv(void);
> +void spapr_unregister_nested_hv(void);
>  
>  #endif /* HW_SPAPR_H */
> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> index d312a5d61d..09d95182b2 100644
> --- a/include/hw/ppc/spapr_nested.h
> +++ b/include/hw/ppc/spapr_nested.h
> @@ -95,7 +95,6 @@ struct nested_ppc_state {
>      int64_t tb_offset;
>  };
>  
> -void spapr_register_nested(void);
>  void spapr_exit_nested(PowerPCCPU *cpu, int excp);
>  
>  #endif /* HW_SPAPR_NESTED_H */
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index e889244e52..f0c2f4de78 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -487,6 +487,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>              error_append_hint(errp, "Try appending -machine cap-nested-hv=off "
>                                      "or use threads=1 with -smp\n");
>          }
> +        spapr_unregister_nested_hv(); /* reset across reboots */
> +        spapr_register_nested_hv();

This looks a bit odd. And actually it also ends up registering them in
the SMT error case (which seems harmelss but a bit inconsistent).

I wonder whether you could make a spapr_nested_reset() function called
by spapr_machine_reset that would take care of
unregistering and reregistering nested hypercalls based on the caps that
were set.

Thanks,
Nick