Re: [Qemu-ppc] [PATCH] target/ppc: Allow privileged access to SPR_PCR

[Cédric Le Goater]

On 05/30/2018 04:42 PM, Joel Stanley wrote:
> The powerpc Linux kernel[1] and skiboot firmware[2] recently gained changes
> that cause the Processor Compatibility Register (PCR) SPR to be cleared.
> 
> These changes cause Linux to fail to boot on the Qemu powernv machine
> with an error:
> 
>  Trying to write privileged spr 338 (0x152) at 0000000030017f0c
> 
> With this patch Qemu makes this register available as a hypervisor
> privileged register.
> 
> Note that bits set in this register disable features of the processor.
> Currently the only register state that is supported is when the register
> is zeroed (enable all features). This is sufficient for guests to
> once again boot.
> 
> [1] https://lkml.kernel.org/r/address@hidden
> [2] https://patchwork.ozlabs.org/patch/915932/
> 
> Signed-off-by: Joel Stanley <address@hidden>
> ---
>  target/ppc/helper.h             |  1 +
>  target/ppc/misc_helper.c        | 10 ++++++++++
>  target/ppc/translate_init.inc.c |  9 +++++++--
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 19453c68138a..d751f0e21909 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -17,6 +17,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
>  DEF_HELPER_1(rfid, void, env)
>  DEF_HELPER_1(hrfid, void, env)
>  DEF_HELPER_2(store_lpcr, void, env, tl)
> +DEF_HELPER_2(store_pcr, void, env, tl)
>  #endif
>  DEF_HELPER_1(check_tlb_flush_local, void, env)
>  DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 8c8cba5cc6f1..40c39d08ad14 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -20,6 +20,7 @@
>  #include "cpu.h"
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
>  
>  #include "helper_regs.h"
>  
> @@ -186,6 +187,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
>      hreg_store_msr(env, value, 0);
>  }
>  
> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
> +{
> +    if (value != 0) {
> +        error_report("Unimplemented PCR value 0x"TARGET_FMT_lx, value);
> +        return;
> +    }
> +    env->spr[SPR_PCR] = value;

shouldn't we use pcc->pcr_mask ? and check pcc->pcr_supported also ? 

C.

> +}
> +
>  /* This code is lifted from MacOnLinux. It is called whenever
>   * THRM1,2 or 3 is read an fixes up the values in such a way
>   * that will make MacOS not hang. These registers exist on some
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ab782cb32aaa..bebe6ec5333c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -456,6 +456,10 @@ static void spr_write_hid0_601(DisasContext *ctx, int 
> sprn, int gprn)
>      /* Must stop the translation as endianness may have changed */
>      gen_stop_exception(ctx);
>  }
> +static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> +{
> +    gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> +}
>  #endif
>  
>  /* Unified bats */
> @@ -7957,11 +7961,12 @@ static void gen_spr_power6_common(CPUPPCState *env)
>  #endif
>      /*
>       * Register PCR to report POWERPC_EXCP_PRIV_REG instead of
> -     * POWERPC_EXCP_INVAL_SPR.
> +     * POWERPC_EXCP_INVAL_SPR in userspace. Permit hypervisor access.
>       */
> -    spr_register(env, SPR_PCR, "PCR",
> +    spr_register_hv(env, SPR_PCR, "PCR",
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   SPR_NOACCESS, SPR_NOACCESS,
> +                 &spr_read_generic, &spr_write_pcr,
>                   0x00000000);
>  }
>  
>