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

[David Gibson]

On Tue, May 02, 2017 at 03:40:37PM +1000, Suraj Jitindar Singh wrote:
> 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;

More or less, yes.

> 
> > 
> > > 
> > > +}
> > > +
> > > +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,
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature