Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V4 3/6] target/ppc: Update tlbie to check privilege level based on GTSE

[Suraj Jitindar Singh]

On Tue, 2017-04-18 at 14:29 +1000, David Gibson wrote:
> On Thu, Apr 13, 2017 at 04:02:37PM +1000, Suraj Jitindar Singh wrote:
> > 
> > The Guest Translation Shootdown Enable (GTSE) bit in the Logical
> > Partition
> > Control Register (LPCR) can be set to enable a guest to use the
> > tlbie
> > instruction directly to invalidate translations.
> > 
> > When the GTSE bit is set then the tlbie instruction is supervisor
> > privileged, otherwise it is hypervisor privileged.
> > 
> > Update the tblie tcg code generation to check the correct privilege
> > level based on the GTSE value in the LPCR.
> > 
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> This can be simplified; in some easy ways and some harder ways.
> 
> 
> Easy ways inline:
> 
> 
> > 
> > 
> > ---
> > 
> > -> V4:
> > 
> > - Added patch to series
> > ---
> >  target/ppc/translate.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index f40b5a1..5f90c03 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4513,7 +4513,27 @@ static void gen_tlbie(DisasContext *ctx)
> >      GEN_PRIV;
> >  #else
> >      TCGv_i32 t1;
> > +    TCGv t2, t3;
> > +    TCGLabel *l1, *l2;
> > +
> > +    l1 = gen_new_label();
> > +    l2 = gen_new_label();
> > +    t2 = tcg_temp_new();
> > +    t3 = tcg_temp_new();
> > +
> > +    tcg_gen_ld_tl(t2, cpu_env, offsetof(CPUPPCState,
> > spr[SPR_LPCR]));
> > +    tcg_gen_andi_tl(t3, t2, LPCR_GTSE);
> You could just re-use t2 as the destination here, thus removing one
> temporary.
> 
> > 
> > +    tcg_temp_free(t2);
> > +    tcg_gen_brcondi_tl(TCG_COND_EQ, t3, 0, l1);
> > +    /* LPCR_GTSE == 1 -> Instruction is Supervisor Privileged */
> > +    tcg_temp_free(t3);
> > +    CHK_SV;
> > +    tcg_gen_br(l2);
> > +    gen_set_label(l1);
> > +    /* LPCR_GTSE == 0 -> Instruction is Hypervisor Privileged */
> > +    tcg_temp_free(t3);
> >      CHK_HV;
> If CHK_HV passes, then CHK_SV certainly will as well, so you can make
> the CHK_SV check unconditional, removing one branch and one label.
> 
> > 
> > +    gen_set_label(l2);
> >  
> >      if (NARROW_MODE(ctx)) {
> >          TCGv t0 = tcg_temp_new();
> 
> Now.. the harder ways.
> 
> If you look at how CHK_HV and CHK_SV are defined, you'll see that
> they
> check the hypervisor / supervisor status at code gen time, rather
> than
> within the generated code - basically the privilege state is used as
> part of the key to look up generated code fragments, so we don't need
> to keep looking at the MSR from generated code.
> 
> Ideally, we would make the GTSE bit the same way - store in the
> DisasContext, so we don't need to look it up from generated
> code.  I'm
> not entirely sure how to do that however.

I think adding this to the DisasContext should be relatively straight
forward... (I'm sure I'll regret saying that). I think I thought of
doing that but went this way instead.
I'll take a closer look.

> 
> What you can do a bit more easily, is to expand out the CHK_HV and
> CHK_SV macros.  For the hypervisor and not-even-supervisor states you
> can do the right thing at codegen time without checking LPCR.  Only
> for the SV && !HV state do you need to generate runtime check of the
> LPCR.
>