Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V4 1/6] target/ppc: Set UPRT and GTSE on all cpus in H_REGISTER_PROCESS_TABLE

[Suraj Jitindar Singh]

On Tue, 2017-04-18 at 13:25 +1000, David Gibson wrote:
> On Thu, Apr 13, 2017 at 04:02:35PM +1000, Suraj Jitindar Singh wrote:
> > 
> > The UPRT and GTSE bits are set when a guest calls
> > H_REGISTER_PROCESS_TABLE
> > to choose determine how address translation is performed. Currently
> > these
> > bits in the LPCR are only set for the cpu which handles the H_CALL,
> > however
> > they need to be set for all cpus for that guest as address
> > translation
> > cannot be performed differently on a per cpu basis.
> Oops, should have spotted that the first time around.  Oh well.
> 
> > 
> > Update the H_CALL handler to set these bits in the LPCR correctly
> > for all
> > cpus of the guest.
> > 
> > Note it is the reponsibility of the guest to ensure that any
> > secondary cpus
> > are suspended when the H_CALL is made and thus we can safely update
> > these
> > values here.
> > 
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > 
> > ---
> > 
> > -> V4:
> > 
> > - Added patch to series
> > ---
> >  hw/ppc/spapr_hcall.c | 49 +++++++++++++++++++++++++++++++++++++---
> > ---------
> >  1 file changed, 37 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 44f4489..fd8f4b4 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -924,6 +924,38 @@ static void
> > spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
> >      return;
> >  }
> >  
> > +typedef struct {
> > +    target_ulong mask;
> > +    bool set;
> > +} SetLPCRState;
> > +
> > +static void do_set_lpcr(CPUState *cs, run_on_cpu_data arg)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    SetLPCRState *s = arg.host_ptr;
> > +
> > +    if (s->set) {
> > +        env->spr[SPR_LPCR] |= s->mask;
> > +    } else {
> > +        env->spr[SPR_LPCR] &= ~s->mask;
> > +    }
> > +}
> I'm finding the mask + set/clear flag a little confusing.
> 
> I'd suggest instead you supply a mask and value, both target_ulong.
> The callback would clear the mask bits, then set the value bits.
> i.e. mask is the bits you want to allow to be changed, value is the
> value to set them too.
> 
> I think that will be a bit clearer, as well as being more flexible.

Sounds good, will do

> 
> > 
> > +
> > +static void ppc_set_lpcr_all(target_ulong mask, bool set)
> > +{
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        SetLPCRState s = {
> > +            .mask = mask,
> > +            .set = set
> > +        };
> > +
> > +        run_on_cpu(cs, do_set_lpcr, RUN_ON_CPU_HOST_PTR(&s));
> > +    }
> > +}
> > +
> >  #define FLAGS_MASK              0x01FULL
> >  #define FLAG_MODIFY             0x10
> >  #define FLAG_REGISTER           0x08
> > @@ -936,7 +968,6 @@ static target_ulong
> > h_register_process_table(PowerPCCPU *cpu,
> >                                               target_ulong opcode,
> >                                               target_ulong *args)
> >  {
> > -    CPUPPCState *env = &cpu->env;
> >      target_ulong flags = args[0];
> >      target_ulong proc_tbl = args[1];
> >      target_ulong page_size = args[2];
> > @@ -992,17 +1023,11 @@ static target_ulong
> > h_register_process_table(PowerPCCPU *cpu,
> >      spapr_check_setup_free_hpt(spapr, spapr->patb_entry, cproc);
> >  
> >      spapr->patb_entry = cproc; /* Save new process table */
> > -    if ((flags & FLAG_RADIX) || (flags & FLAG_HASH_PROC_TBL)) {
> > -        /* Use Process TBL */
> > -        env->spr[SPR_LPCR] |= LPCR_UPRT;
> > -    } else {
> > -        env->spr[SPR_LPCR] &= ~LPCR_UPRT;
> > -    }
> > -    if (flags & FLAG_GTSE) { /* Partition Uses Guest Translation
> > Shootdwn */
> > -        env->spr[SPR_LPCR] |= LPCR_GTSE;
> > -    } else {
> > -        env->spr[SPR_LPCR] &= ~LPCR_GTSE;
> > -    }
> > +
> > +    /* Update the UPRT and GTSE bits in the LPCR for all cpus */
> > +    ppc_set_lpcr_all(LPCR_UPRT, (flags & FLAG_RADIX) ||
> > +                                (flags & FLAG_HASH_PROC_TBL));
> > +    ppc_set_lpcr_all(LPCR_GTSE, !!(flags & FLAG_GTSE));
> >  
> >      if (kvm_enabled()) {
> >          return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,