Re: [PATCH v4 09/15] spapr: nested: Extend nested_ppc_state for nested PAPR API

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

On 2/27/24 15:29, Nicholas Piggin wrote:
> On Tue Feb 20, 2024 at 6:36 PM AEST, Harsh Prateek Bora wrote:
>> Currently, nested_ppc_state stores a certain set of registers and works
>> with nested_[load|save]_state() for state transfer as reqd for nested-hv API.
>> Extending these with additional registers state as reqd for nested PAPR API.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   include/hw/ppc/spapr_nested.h |  49 ++++++++++++++++
>>   target/ppc/cpu.h              |   2 +
>>   hw/ppc/spapr_nested.c         | 106 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 157 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
>> index 24e87bca08..a3b61eb79a 100644
>> --- a/include/hw/ppc/spapr_nested.h
>> +++ b/include/hw/ppc/spapr_nested.h
>> @@ -7,6 +7,7 @@ typedef struct SpaprMachineStateNested {
>>       uint64_t ptcr;
>>       uint8_t api;
>>   #define NESTED_API_KVM_HV  1
>> +#define NESTED_API_PAPR    2
>>       bool capabilities_set;
>>       uint32_t pvr_base;
>>       GHashTable *guests;
>> @@ -123,6 +124,54 @@ struct nested_ppc_state {
>>       int64_t tb_offset;
>>       /* Nested PAPR API */
>>       uint64_t pvr;
>> +    uint64_t amor;
>> +    uint64_t dawr0;
>> +    uint64_t dawrx0;
>> +    uint64_t ciabr;
>> +    uint64_t purr;
>> +    uint64_t spurr;
>> +    uint64_t ic;
>> +    uint64_t vtb;
>> +    uint64_t hdar;
>> +    uint64_t hdsisr;
>> +    uint64_t heir;
>> +    uint64_t asdr;
>> +    uint64_t dawr1;
>> +    uint64_t dawrx1;
>> +    uint64_t dexcr;
>> +    uint64_t hdexcr;
>> +    uint64_t hashkeyr;
>> +    uint64_t hashpkeyr;
>> +    ppc_vsr_t vsr[64] QEMU_ALIGNED(16);
>> +    uint64_t ebbhr;
>> +    uint64_t tar;
>> +    uint64_t ebbrr;
>> +    uint64_t bescr;
>> +    uint64_t iamr;
>> +    uint64_t amr;
>> +    uint64_t uamor;
>> +    uint64_t dscr;
>> +    uint64_t fscr;
>> +    uint64_t pspb;
>> +    uint64_t ctrl;
>> +    uint64_t vrsave;
>> +    uint64_t dar;
>> +    uint64_t dsisr;
>> +    uint64_t pmc1;
>> +    uint64_t pmc2;
>> +    uint64_t pmc3;
>> +    uint64_t pmc4;
>> +    uint64_t pmc5;
>> +    uint64_t pmc6;
>> +    uint64_t mmcr0;
>> +    uint64_t mmcr1;
>> +    uint64_t mmcr2;
>> +    uint64_t mmcra;
>> +    uint64_t sdar;
>> +    uint64_t siar;
>> +    uint64_t sier;
>> +    uint32_t vscr;
>> +    uint64_t fpscr;
>>   };
>>   
>>   typedef struct SpaprMachineStateNestedGuestVcpu {
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index a44de22ca4..11205bb9e3 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1737,9 +1737,11 @@ void ppc_compat_add_property(Object *obj, const char *name,
>>   #define SPR_PSPB              (0x09F)
>>   #define SPR_DPDES             (0x0B0)
>>   #define SPR_DAWR0             (0x0B4)
>> +#define SPR_DAWR1             (0x0B5)
>>   #define SPR_RPR               (0x0BA)
>>   #define SPR_CIABR             (0x0BB)
>>   #define SPR_DAWRX0            (0x0BC)
>> +#define SPR_DAWRX1            (0x0BD)
>>   #define SPR_HFSCR             (0x0BE)
>>   #define SPR_VRSAVE            (0x100)
>>   #define SPR_USPRG0            (0x100)
> 
> Might try to put the DAWR1 enable ahead of this, but if not we'll have
> to drop these until that is done. Leave it in for now I'll sort it out
> if necessary.

Ok

> 
>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>> index 3cc704adda..39d0c087f1 100644
>> --- a/hw/ppc/spapr_nested.c
>> +++ b/hw/ppc/spapr_nested.c
>> @@ -101,6 +101,7 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>>   static void nested_save_state(struct nested_ppc_state *save, PowerPCCPU *cpu)
>>   {
>>       CPUPPCState *env = &cpu->env;
>> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> 
> I hope that won't be a big overhead... nested entry/exit performance probably
> isn't top priority at the moment though, so for now okay. Should be
> something to look at though.

Hmm.. Ok

> 
> 
>>       memcpy(save->gpr, env->gpr, sizeof(save->gpr));
>>   
>> @@ -127,6 +128,58 @@ static void nested_save_state(struct nested_ppc_state *save, PowerPCCPU *cpu)
>>       save->pidr = env->spr[SPR_BOOKS_PID];
>>       save->ppr = env->spr[SPR_PPR];
>>   
>> +    if (spapr_nested_api(spapr) == NESTED_API_PAPR) {
>> +        save->pvr = env->spr[SPR_PVR];
>> +        save->amor = env->spr[SPR_AMOR];
>> +        save->dawr0 = env->spr[SPR_DAWR0];
>> +        save->dawrx0 = env->spr[SPR_DAWRX0];
>> +        save->ciabr = env->spr[SPR_CIABR];
>> +        save->purr = env->spr[SPR_PURR];
>> +        save->spurr = env->spr[SPR_SPURR];
>> +        save->ic = env->spr[SPR_IC];
>> +        save->vtb = env->spr[SPR_VTB];
>> +        save->hdar = env->spr[SPR_HDAR];
>> +        save->hdsisr = env->spr[SPR_HDSISR];
>> +        save->heir = env->spr[SPR_HEIR];
>> +        save->asdr = env->spr[SPR_ASDR];
>> +        save->dawr1 = env->spr[SPR_DAWR1];
>> +        save->dawrx1 = env->spr[SPR_DAWRX1];
>> +        save->dexcr = env->spr[SPR_DEXCR];
>> +        save->hdexcr = env->spr[SPR_HDEXCR];
>> +        save->hashkeyr = env->spr[SPR_HASHKEYR];
>> +        save->hashpkeyr = env->spr[SPR_HASHPKEYR];
>> +        memcpy(save->vsr, env->vsr, sizeof(save->vsr));
>> +        save->ebbhr = env->spr[SPR_EBBHR];
>> +        save->tar = env->spr[SPR_TAR];
>> +        save->ebbrr = env->spr[SPR_EBBRR];
>> +        save->bescr = env->spr[SPR_BESCR];
>> +        save->iamr = env->spr[SPR_IAMR];
>> +        save->amr = env->spr[SPR_AMR];
>> +        save->uamor = env->spr[SPR_UAMOR];
>> +        save->dscr = env->spr[SPR_DSCR];
>> +        save->fscr = env->spr[SPR_FSCR];
>> +        save->pspb = env->spr[SPR_PSPB];
>> +        save->ctrl = env->spr[SPR_CTRL];
>> +        save->vrsave = env->spr[SPR_VRSAVE];
>> +        save->dar = env->spr[SPR_DAR];
>> +        save->dsisr = env->spr[SPR_DSISR];
>> +        save->pmc1 = env->spr[SPR_POWER_PMC1];
>> +        save->pmc2 = env->spr[SPR_POWER_PMC2];
>> +        save->pmc3 = env->spr[SPR_POWER_PMC3];
>> +        save->pmc4 = env->spr[SPR_POWER_PMC4];
>> +        save->pmc5 = env->spr[SPR_POWER_PMC5];
>> +        save->pmc6 = env->spr[SPR_POWER_PMC6];
>> +        save->mmcr0 = env->spr[SPR_POWER_MMCR0];
>> +        save->mmcr1 = env->spr[SPR_POWER_MMCR1];
>> +        save->mmcr2 = env->spr[SPR_POWER_MMCR2];
>> +        save->mmcra = env->spr[SPR_POWER_MMCRA];
>> +        save->sdar = env->spr[SPR_POWER_SDAR];
>> +        save->siar = env->spr[SPR_POWER_SIAR];
>> +        save->sier = env->spr[SPR_POWER_SIER];
>> +        save->vscr = ppc_get_vscr(env);
>> +        save->fpscr = env->fpscr;
>> +    }
>> +
>>       save->tb_offset = env->tb_env->tb_offset;
>>   }
>>   
>> @@ -134,6 +187,7 @@ static void nested_load_state(PowerPCCPU *cpu, struct nested_ppc_state *load)
>>   {
>>       CPUState *cs = CPU(cpu);
>>       CPUPPCState *env = &cpu->env;
>> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>   
>>       memcpy(env->gpr, load->gpr, sizeof(env->gpr));
>>   
>> @@ -160,6 +214,58 @@ static void nested_load_state(PowerPCCPU *cpu, struct nested_ppc_state *load)
>>       env->spr[SPR_BOOKS_PID] = load->pidr;
>>       env->spr[SPR_PPR] = load->ppr;
>>   
>> +    if (spapr_nested_api(spapr) == NESTED_API_PAPR) {
>> +        env->spr[SPR_PVR] = load->pvr;
>> +        env->spr[SPR_AMOR] = load->amor;
>> +        env->spr[SPR_DAWR0] = load->dawr0;
>> +        env->spr[SPR_DAWRX0] = load->dawrx0;
>> +        env->spr[SPR_CIABR] = load->ciabr;
>> +        env->spr[SPR_PURR] = load->purr;
>> +        env->spr[SPR_SPURR] = load->purr;
>> +        env->spr[SPR_IC] = load->ic;
>> +        env->spr[SPR_VTB] = load->vtb;
>> +        env->spr[SPR_HDAR] = load->hdar;
>> +        env->spr[SPR_HDSISR] = load->hdsisr;
>> +        env->spr[SPR_HEIR] = load->heir;
>> +        env->spr[SPR_ASDR] = load->asdr;
>> +        env->spr[SPR_DAWR1] = load->dawr1;
>> +        env->spr[SPR_DAWRX1] = load->dawrx1;
>> +        env->spr[SPR_DEXCR] = load->dexcr;
>> +        env->spr[SPR_HDEXCR] = load->hdexcr;
>> +        env->spr[SPR_HASHKEYR] = load->hashkeyr;
>> +        env->spr[SPR_HASHPKEYR] = load->hashpkeyr;
>> +        memcpy(env->vsr, load->vsr, sizeof(env->vsr));
>> +        env->spr[SPR_EBBHR] = load->ebbhr;
>> +        env->spr[SPR_TAR] = load->tar;
>> +        env->spr[SPR_EBBRR] = load->ebbrr;
>> +        env->spr[SPR_BESCR] = load->bescr;
>> +        env->spr[SPR_IAMR] = load->iamr;
>> +        env->spr[SPR_AMR] = load->amr;
>> +        env->spr[SPR_UAMOR] = load->uamor;
>> +        env->spr[SPR_DSCR] = load->dscr;
>> +        env->spr[SPR_FSCR] = load->fscr;
>> +        env->spr[SPR_PSPB] = load->pspb;
>> +        env->spr[SPR_CTRL] = load->ctrl;
>> +        env->spr[SPR_VRSAVE] = load->vrsave;
>> +        env->spr[SPR_DAR] = load->dar;
>> +        env->spr[SPR_DSISR] = load->dsisr;
>> +        env->spr[SPR_POWER_PMC1] = load->pmc1;
>> +        env->spr[SPR_POWER_PMC2] = load->pmc2;
>> +        env->spr[SPR_POWER_PMC3] = load->pmc3;
>> +        env->spr[SPR_POWER_PMC4] = load->pmc4;
>> +        env->spr[SPR_POWER_PMC5] = load->pmc5;
>> +        env->spr[SPR_POWER_PMC6] = load->pmc6;
>> +        env->spr[SPR_POWER_MMCR0] = load->mmcr0;
>> +        env->spr[SPR_POWER_MMCR1] = load->mmcr1;
>> +        env->spr[SPR_POWER_MMCR2] = load->mmcr2;
>> +        env->spr[SPR_POWER_MMCRA] = load->mmcra;
> 
> I worry that some other SPRs have "behaviour" we need
> to handle. DAWR and MMCR? for example set breakpoints
> and PMU beahviour. I don't know how well that is handled
> here.

I think for now we can keep it as-is. Any additional handling can be
taken care as separate patches in future?

regards,
Harsh

> 
> Thanks,
> Nick