Re: [RFC PATCH 2/4] spapr: Add a nested state struct

[Nicholas Piggin]

On Fri May 5, 2023 at 8:54 PM AEST, Harsh Prateek Bora wrote:
> <correcting my email in CC>
>
> On 5/3/23 06:09, Nicholas Piggin wrote:
> > @@ -1593,12 +1713,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >           return H_PARAMETER;
> >       }
> >   
> > -    spapr_cpu->nested_host_state = g_try_new(CPUPPCState, 1);
> > +    spapr_cpu->nested_host_state = g_try_new(struct nested_ppc_state, 1);
> >       if (!spapr_cpu->nested_host_state) {
> >           return H_NO_MEM;
> >       }
> >   
> > -    memcpy(spapr_cpu->nested_host_state, env, sizeof(CPUPPCState));
> > +    assert(env->spr[SPR_LPIDR] == 0);
> > +    assert(env->spr[SPR_DPDES] == 0);
> > +    nested_save_state(spapr_cpu->nested_host_state, cpu);
> >   
> Ideally, we may want to save entire env for L1 host, while switching to 
> L2 rather than keeping a subset of it for 2 reasons:
>   - keeping enitre L1 env ensures it remains untouched by L2 during L2 
> execution (shouldnt allow L2 to modify remaining L1 env bits unexpectedly)

It doesn't because you need to restore it, and you can't just restore
all. Making it symmetrical and saving what you restore is better. It's
a pretty nasty layering violation to memcpy the whole CPUPPCState too,
conceptually.

I prefer that we have to think about each bit of state that is changed
and how we deal with it -- I'd not be at all surprised if there are bits
that are wrong as is, e.g., in interrupt handling.

>   - I see some of the registers are retained only for L1 (so, ca, ov32, 
> ca32, etc) but not for L2 (got missed in nested_load_state helper in 
> this patch), are they not really needed anymore? Previous patch 
> introduced one of them though.

I'm not sure. those fields aren't registers as such, but mirror in some
values from regisers. I didn't think I got it wrong but I'll double
check.

Thanks,
Nick