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

[Suraj Jitindar Singh]

On Mon, 2017-05-01 at 16:59 +1000, David Gibson wrote:
> On Fri, Apr 28, 2017 at 04:58:21PM +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.
> > 
> > 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 -> V5:
> > - Rework spr setting code for extensibility
> > 
> > -> V4:
> > - Added patch to series
> > ---
> >  hw/ppc/spapr_hcall.c | 51 +++++++++++++++++++++++++++++++++++++++-
> > -----------
> >  1 file changed, 39 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 44f4489..2e571cc 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 {
> > +    int spr_n;
> > +    target_ulong mask;
> > +    target_ulong val;
> > +} SetSPRState;
> > +
> > +static void do_set_spr(CPUState *cs, run_on_cpu_data arg)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    SetSPRState *s = arg.host_ptr;
> > +
> > +    env->spr[s->spr_n] &= ~s->mask;
> > +    env->spr[s->spr_n] |= (s->val & s->mask);
> I guess since it's running on the cpu thread this will be atomic
> enough.  Personally I'd use a local for the intermediate value,
> though.

You mean something like:

target_ulong val;

val = env->spr[s->spr_n] & ~s->mask;
val |= s->val & s->mask;

env->spr[s->spr_n] = val;

> 
> > 
> > +}
> > +
> > +static void ppc_set_spr_cpu_foreach(int spr_n, target_ulong mask,
> > +                                    target_ulong val)
> > +{
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        SetSPRState s = {
> > +            .spr_n = spr_n,
> > +            .mask = mask,
> > +            .val = val
> > +        };
> > +
> > +        run_on_cpu(cs, do_set_spr, 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,13 @@ 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_spr_cpu_foreach(SPR_LPCR, LPCR_UPRT,
> > +                            (flags & (FLAG_RADIX |
> > FLAG_HASH_PROC_TBL)) ?
> > +                            LPRC_UPRT : 0);
> > +    ppc_set_spr_cpu_foreach(SPR_LPCR, LPCR_GTSE, (flags &
> > FLAG_GTSE) ? LPCR_GTSE
> > +                                                                  
> >    : 0);
> You should be able to set both flags with a single foreach, no?

Yeap, my bad

> 
> > 
> >  
> >      if (kvm_enabled()) {
> >          return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,