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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

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 */
>