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

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

[Joel Stanley]

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/20180518013742.24095-1-mikey@neuling.org
[2] https://patchwork.ozlabs.org/patch/915932/

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
 - Change error message to say Invalid instead of Unimplemented
 - Fix compile warning on other powerpc targets, thanks patchew
---
 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..1870f4c47f1a 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"
 
@@ -98,6 +99,15 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
         tlb_flush(CPU(cpu));
     }
 }
+
+void helper_store_pcr(CPUPPCState *env, target_ulong value)
+{
+    if (value != 0) {
+        error_report("Invalid PCR value 0x"TARGET_FMT_lx, value);
+        return;
+    }
+    env->spr[SPR_PCR] = value;
+}
 #endif /* defined(TARGET_PPC64) */
 
 void helper_store_pidr(CPUPPCState *env, target_ulong val)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index ab782cb32aaa..1a89017ddea8 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -424,6 +424,10 @@ static void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn)
     gen_helper_store_ptcr(cpu_env, cpu_gpr[gprn]);
 }
 
+static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
+{
+    gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
+}
 #endif
 #endif
 
@@ -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);
 }
 
-- 
2.17.0

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

[Greg Kurz]

On Thu, 31 May 2018 22:34:29 +0930
Joel Stanley <joel@jms.id.au> 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/20180518013742.24095-1-mikey@neuling.org
> [2] https://patchwork.ozlabs.org/patch/915932/
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Change error message to say Invalid instead of Unimplemented
>  - Fix compile warning on other powerpc targets, thanks patchew
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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..1870f4c47f1a 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"
>  
> @@ -98,6 +99,15 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
>          tlb_flush(CPU(cpu));
>      }
>  }
> +
> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
> +{
> +    if (value != 0) {
> +        error_report("Invalid PCR value 0x"TARGET_FMT_lx, value);
> +        return;
> +    }
> +    env->spr[SPR_PCR] = value;
> +}
>  #endif /* defined(TARGET_PPC64) */
>  
>  void helper_store_pidr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ab782cb32aaa..1a89017ddea8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -424,6 +424,10 @@ static void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn)
>      gen_helper_store_ptcr(cpu_env, cpu_gpr[gprn]);
>  }
>  
> +static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> +{
> +    gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> +}
>  #endif
>  #endif
>  
> @@ -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);
>  }
>  

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4082 bytes --]

On Thu, May 31, 2018 at 10:34:29PM +0930, 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/20180518013742.24095-1-mikey@neuling.org
> [2] https://patchwork.ozlabs.org/patch/915932/
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Change error message to say Invalid instead of Unimplemented
>  - Fix compile warning on other powerpc targets, thanks patchew
> ---
>  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..1870f4c47f1a 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"
>  
> @@ -98,6 +99,15 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
>          tlb_flush(CPU(cpu));
>      }
>  }
> +
> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
> +{
> +    if (value != 0) {
> +        error_report("Invalid PCR value 0x"TARGET_FMT_lx, value);
> +        return;
> +    }

I don't see a lot of point to this check.  For now the only user just
clears the PCR, but why limit yourself to that explicitly?

> +    env->spr[SPR_PCR] = value;

You definitely should be masking against pcr_mask too.

> +}
>  #endif /* defined(TARGET_PPC64) */
>  
>  void helper_store_pidr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ab782cb32aaa..1a89017ddea8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -424,6 +424,10 @@ static void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn)
>      gen_helper_store_ptcr(cpu_env, cpu_gpr[gprn]);
>  }
>  
> +static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> +{
> +    gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> +}
>  #endif
>  #endif
>  
> @@ -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);
>  }
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Joel Stanley]

On 4 June 2018 at 17:34, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, May 31, 2018 at 10:34:29PM +0930, Joel Stanley wrote:
>> @@ -98,6 +99,15 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
>>          tlb_flush(CPU(cpu));
>>      }
>>  }
>> +
>> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
>> +{
>> +    if (value != 0) {
>> +        error_report("Invalid PCR value 0x"TARGET_FMT_lx, value);
>> +        return;
>> +    }
>
> I don't see a lot of point to this check.  For now the only user just
> clears the PCR, but why limit yourself to that explicitly?

As we're not emulating the features of the register when the value is
non-zero, I thought it would make make sense to do something.

I'll drop it in v3.

>
>> +    env->spr[SPR_PCR] = value;
>
> You definitely should be masking against pcr_mask too.

Okay. Thanks for the review.

Cheers,

Joel

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Mon, Jun 04, 2018 at 06:06:04PM +0930, Joel Stanley wrote:
> On 4 June 2018 at 17:34, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Thu, May 31, 2018 at 10:34:29PM +0930, Joel Stanley wrote:
> >> @@ -98,6 +99,15 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
> >>          tlb_flush(CPU(cpu));
> >>      }
> >>  }
> >> +
> >> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
> >> +{
> >> +    if (value != 0) {
> >> +        error_report("Invalid PCR value 0x"TARGET_FMT_lx, value);
> >> +        return;
> >> +    }
> >
> > I don't see a lot of point to this check.  For now the only user just
> > clears the PCR, but why limit yourself to that explicitly?
> 
> As we're not emulating the features of the register when the value is
> non-zero, I thought it would make make sense to do something.

Ah.. I see what you mean.

Ok, the check makes sense in that case - but it needs a comment saying
it should go away once we actually put the proper PCR checks in the
various bits of TCG that should have them.

> 
> I'll drop it in v3.
> 
> >
> >> +    env->spr[SPR_PCR] = value;
> >
> > You definitely should be masking against pcr_mask too.
> 
> Okay. Thanks for the review.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]