On 9/7/23 07:39, 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_SET_CAPABILITIES.
>> This is used by L1 to set capabilities of the nested guest being
>> created. The capabilities being set are subset of the capabilities
>> returned from the previous call to H_GUEST_GET_CAPABILITIES hcall.
>> Currently, it only supports P9/P10 capability check through PVR.
>>
>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>> hw/ppc/spapr.c | 1 +
>> hw/ppc/spapr_nested.c | 46 +++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr_nested.h | 3 +++
>> 3 files changed, 50 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index cbab7a825f..7c6f6ee25d 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3443,6 +3443,7 @@ static void spapr_instance_init(Object *obj)
>> "Host serial number to advertise in guest device tree");
>> /* Nested */
>> spapr->nested.api = 0;
>> + spapr->nested.capabilities_set = false;
>
> I would actually think about moving spapr->nested init into
> spapr_nested.c.
>
Agree, moved.
>> }
>>
>> static void spapr_machine_finalizefn(Object *obj)
>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>> index 37f3a49be2..9af65f257f 100644
>> --- a/hw/ppc/spapr_nested.c
>> +++ b/hw/ppc/spapr_nested.c
>> @@ -399,6 +399,51 @@ static target_ulong h_guest_get_capabilities(PowerPCCPU *cpu,
>> return H_SUCCESS;
>> }
>>
>> +static target_ulong h_guest_set_capabilities(PowerPCCPU *cpu,
>> + SpaprMachineState *spapr,
>> + target_ulong opcode,
>> + target_ulong *args)
>> +{
>> + CPUPPCState *env = &cpu->env;
>> + target_ulong flags = args[0];
>> + target_ulong capabilities = args[1];
>> +
>> + if (flags) { /* don't handle any flags capabilities for now */
>> + return H_PARAMETER;
>> + }
>> +
>> +
>
> May need to do a pass over whitespace.
>
Sure, done.
>> + /* isn't supported */
>> + if (capabilities & H_GUEST_CAPABILITIES_COPY_MEM) {
>> + env->gpr[4] = 0;
>> + return H_P2;
>> + }
>> +
>> + if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
>> + (CPU_POWERPC_POWER9_BASE)) {
>> + /* We are a P9 */
>> + if (!(capabilities & H_GUEST_CAPABILITIES_P9_MODE)) {
>> + env->gpr[4] = 1;
>> + return H_P2;
>> + }
>> + }
>> +
>> + if ((env->spr[SPR_PVR] & CPU_POWERPC_POWER_SERVER_MASK) ==
>> + (CPU_POWERPC_POWER10_BASE)) {
>> + /* We are a P10 */
>
> The 2 comments above aren't helpful. Just remove them.
>
Sure, done.
>> + if (!(capabilities & H_GUEST_CAPABILITIES_P10_MODE)) {
>> + env->gpr[4] = 2;
>> + return H_P2;
>> + }
>> + }
>> +
>> + spapr->nested.capabilities_set = true;
>
> Is it okay to set twice? If not, add a check. If yes, remove
> capabilities_set until it's needed.
>
Thanks for pointing it out, adding a check as appropriate.
Thanks
Harsh
>> +
>> + spapr->nested.pvr_base = env->spr[SPR_PVR];
>> +
>> + return H_SUCCESS;
>> +}
>> +
>> void spapr_register_nested(void)
>> {
>> spapr_register_hypercall(KVMPPC_H_SET_PARTITION_TABLE, h_set_ptbl);
>> @@ -410,6 +455,7 @@ void spapr_register_nested(void)
>> void spapr_register_nested_phyp(void)
>> {
>> spapr_register_hypercall(H_GUEST_GET_CAPABILITIES, h_guest_get_capabilities);
>> + spapr_register_hypercall(H_GUEST_SET_CAPABILITIES, h_guest_set_capabilities);
>> }
>>
>> #else
>> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
>> index ce198e9f70..a7996251cb 100644
>> --- a/include/hw/ppc/spapr_nested.h
>> +++ b/include/hw/ppc/spapr_nested.h
>> @@ -193,6 +193,9 @@
>> #define H_GUEST_CAPABILITIES_COPY_MEM 0x8000000000000000
>> #define H_GUEST_CAPABILITIES_P9_MODE 0x4000000000000000
>> #define H_GUEST_CAPABILITIES_P10_MODE 0x2000000000000000
>> +#define H_GUEST_CAP_COPY_MEM_BMAP 0
>> +#define H_GUEST_CAP_P9_MODE_BMAP 1
>> +#define H_GUEST_CAP_P10_MODE_BMAP 2
>>
>> typedef struct SpaprMachineStateNestedGuest {
>> unsigned long vcpus;
>