[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V5 1/6] target/ppc: Set UPRT an
From: |
Suraj Jitindar Singh |
Subject: |
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 |
Date: |
Tue, 02 May 2017 15:53:00 +1000 |
On Tue, 2017-05-02 at 15:40 +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;
Turns out I reinvented the wheel. There is pretty much identical code
at the top of this file which I somehow completely missed...
Yeah woops
>
> >
> >
> > >
> > >
> > > +}
> > > +
> > > +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,