Re: [PATCH RESEND 06/15] ppc: spapr: Implement nested PAPR hcall - H_GUEST_GET_CAPABILITIES

[Harsh Prateek Bora]

On 9/7/23 07:32, Nicholas Piggin wrote:
> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> > This patch implements nested PAPR hcall H_GUEST_GET_CAPABILITIES and
> > also enables registration of nested PAPR hcalls whenever an L0 is
> > launched with cap-nested-papr=true. The common registration routine
> > shall be used by future patches for registration of related hcall
> > support
> > being added. This hcall is used by L1 kernel to get the set of guest
> > capabilities that are supported by L0 (Qemu TCG).
>
> Changelog can drop "This patch". Probably don't have to be so
> detailed here either -- we already established that PAPR hcalls can
> be used with cap-nested-papr in the last patch, we know that L1
> kernels make the hcalls to the vhyp, etc.
>
> "Introduce the nested PAPR hcall H_GUEST_GET_CAPABILITIES which
> is used to query the capabilities of the API and the L2 guests
> it provides."
>
> I would squash this with set.

Sure, will update the commit log and squash with set.

> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_caps.c           |  1 +
> >   hw/ppc/spapr_nested.c         | 35 +++++++++++++++++++++++++++++++++++
> >   include/hw/ppc/spapr_nested.h |  6 ++++++
> >   3 files changed, 42 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index d3b9f107aa..cbe53a79ec 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -511,6 +511,7 @@ static void cap_nested_papr_apply(SpaprMachineState *spapr,
> >               return;
> >           }
> >           spapr->nested.api = NESTED_API_PAPR;
> > +        spapr_register_nested_phyp();
> >       } else if (kvm_enabled()) {
> >           /*
> >            * this gets executed in L1 qemu when L2 is launched,
>
> Hmm, this doesn't match nested HV registration. If you want to register
> the hcalls in the cap apply, can you move spapr_register_nested()
> there first? It may make more sense to go in as a dummy function with
> the cap patch first, since you don't introduce all hcalls together.
>
> Also phyp->papr. Scrub for phyp please.

Sure, will do.

> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index a669470f1a..37f3a49be2 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -6,6 +6,7 @@
> >   #include "hw/ppc/spapr.h"
> >   #include "hw/ppc/spapr_cpu_core.h"
> >   #include "hw/ppc/spapr_nested.h"
> > +#include "cpu-models.h"
> >   
> >   #ifdef CONFIG_TCG
> >   #define PRTS_MASK      0x1f
> > @@ -375,6 +376,29 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> >       address_space_unmap(CPU(cpu)->as, regs, len, len, true);
> >   }
> >   
> > +static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
> > +                                             SpaprMachineState *spapr,
> > +                                             target_ulong opcode,
> > +                                             target_ulong *args)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +    target_ulong flags = args[0];
> > +
> > +    if (flags) { /* don't handle any flags capabilities for now */
> > +        return H_PARAMETER;
> > +    }
> > +
> > +    if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
> > +        (CPU_POWERPC_POWER9_BASE))
> > +        env->gpr[4] = H_GUEST_CAPABILITIES_P9_MODE;
> > +
> > +    if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
> > +        (CPU_POWERPC_POWER10_BASE))
> > +        env->gpr[4] = H_GUEST_CAPABILITIES_P10_MODE;
> > +
> > +    return H_SUCCESS;
> > +}
> > +
> >   void spapr_register_nested(void)
> >   {
> >       spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
> > @@ -382,6 +406,12 @@ void spapr_register_nested(void)
> >       spapr_register_hypercall(KVMPPC_H_TLB_INVALIDATE, h_tlb_invalidate);
> >       spapr_register_hypercall(KVMPPC_H_COPY_TOFROM_GUEST, h_copy_tofrom_guest);
> >   }
> > +
> > +void spapr_register_nested_phyp(void)
> > +{
> > +    spapr_register_hypercall(H_GUEST_GET_CAPABILITIES, 
> > h_guest_get_capabilities);
> > +}
> > +
> >   #else
> >   void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> >   {
> > @@ -392,4 +422,9 @@ void spapr_register_nested(void)
> >   {
> >       /* DO NOTHING */
> >   }
> > +
> > +void spapr_register_nested_phyp(void)
> > +{
> > +    /* DO NOTHING */
> > +}
> >   #endif
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index f8db31075b..ce198e9f70 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -189,6 +189,11 @@
> >   /* End of list of Guest State Buffer Element IDs */
> >   #define GSB_LAST                GSB_VCPU_SPR_ASDR
> >   
> > +/* Bit masks to be used in nested PAPR API */
> > +#define H_GUEST_CAPABILITIES_COPY_MEM 0x8000000000000000
> > +#define H_GUEST_CAPABILITIES_P9_MODE  0x4000000000000000
> > +#define H_GUEST_CAPABILITIES_P10_MODE 0x2000000000000000
>
> See introducing these defines with the patch that uses them isn't so
> bad :)

It's better indeed:)

regards,
Harsh

> Thanks,
> Nick
>
> > +
> >   typedef struct SpaprMachineStateNestedGuest {
> >       unsigned long vcpus;
> >       struct SpaprMachineStateNestedGuestVcpu *vcpu;
> > @@ -331,6 +336,7 @@ struct nested_ppc_state {
> >   };
> >   
> >   void spapr_register_nested(void);
> > +void spapr_register_nested_phyp(void);
> >   void spapr_exit_nested(PowerPCCPU *cpu, int excp);
> >   
> >   #endif /* HW_SPAPR_NESTED_H */