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

[David Gibson]

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.

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.

-- 
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