Re: [PATCH RESEND 14/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_DELETE

["Nicholas Piggin" <npiggin@gmail.com>]

On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> This hcall is used by L1 to delete a guest entry in L0 or can also be
> used to delete all guests if needed (usually in shutdown scenarios).

I'd squash with at least the create hcall.

>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  hw/ppc/spapr_nested.c         | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_nested.h |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> index 3605f27115..5afdad4990 100644
> --- a/hw/ppc/spapr_nested.c
> +++ b/hw/ppc/spapr_nested.c
> @@ -1692,6 +1692,37 @@ static void exit_process_output_buffer(PowerPCCPU *cpu,
>      return;
>  }
>  
> +static target_ulong h_guest_delete(PowerPCCPU *cpu,
> +                                   SpaprMachineState *spapr,
> +                                   target_ulong opcode,
> +                                   target_ulong *args)
> +{
> +    target_ulong flags = args[0];
> +    target_ulong lpid = args[1];
> +    struct SpaprMachineStateNestedGuest *guest;
> +
> +    if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_PAPR)) {
> +        return H_FUNCTION;
> +    }

If you only register these hcalls when you apply the cap, then you
don't need to test it, right?

Open question as to whether it's better to register hcalls when
enabling such caps, or do the tests for them here. I guess the
former makes sense.

> +
> +    /* handle flag deleteAllGuests, remaining bits reserved */

This comment is confusing. What is flag deleteAllGuests?

H_GUEST_DELETE_ALL_MASK? Is that a mask, or a flag?

> +    if (flags & ~H_GUEST_DELETE_ALL_MASK) {
> +        return H_UNSUPPORTED_FLAG;
> +    } else if (flags & H_GUEST_DELETE_ALL_MASK) {
> +        g_hash_table_destroy(spapr->nested.guests);
> +        return H_SUCCESS;
> +    }
> +
> +    guest = g_hash_table_lookup(spapr->nested.guests, GINT_TO_POINTER(lpid));
> +    if (!guest) {
> +        return H_P2;
> +    }
> +
> +    g_hash_table_remove(spapr->nested.guests, GINT_TO_POINTER(lpid));
> +
> +    return H_SUCCESS;
> +}
> +
>  void spapr_register_nested(void)
>  {
>      spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> @@ -1709,6 +1740,7 @@ void spapr_register_nested_phyp(void)
>      spapr_register_hypercall(H_GUEST_SET_STATE       , h_guest_set_state);
>      spapr_register_hypercall(H_GUEST_GET_STATE       , h_guest_get_state);
>      spapr_register_hypercall(H_GUEST_RUN_VCPU        , h_guest_run_vcpu);
> +    spapr_register_hypercall(H_GUEST_DELETE          , h_guest_delete);
>  }
>  
>  #else
> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> index ca5d28c06e..9eb43778ad 100644
> --- a/include/hw/ppc/spapr_nested.h
> +++ b/include/hw/ppc/spapr_nested.h
> @@ -209,6 +209,7 @@
>  #define H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE 0x8000000000000000 /* BE in GSB */
>  #define GUEST_STATE_REQUEST_GUEST_WIDE       0x1
>  #define GUEST_STATE_REQUEST_SET              0x2
> +#define H_GUEST_DELETE_ALL_MASK              0x8000000000000000ULL
>  
>  #define GUEST_STATE_ELEMENT(i, sz, s, f, ptr, c) { \
>      .id = (i),                                     \