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

[Joel Stanley]

On 31 May 2018 at 21:43, Greg Kurz <address@hidden> wrote:
> On Thu, 31 May 2018 21:28:08 +0930
> Joel Stanley <address@hidden> wrote:
>
>> On 31 May 2018 at 20:57, Greg Kurz <address@hidden> wrote:
>> > On Thu, 31 May 2018 09:38:10 +0200
>> > Cédric Le Goater <address@hidden> wrote:
>> >
>> >> 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 ?
>> >>
>> >
>> > pcc->pcr_mask and ppc->pcr_supported only make sense for pseries machine
>> > types (ie, when the spapr machine code call ppc_*_compat() functions).
>> >
>> > The case here is different: we're running a fully emulated pnv machine,
>> > ie, PCR can only be set by mtspr() called within the pnv guest. But TCG
>> > doesn't implement the compatibility mode logic, ie, the CPU always run
>> > in "raw" mode, ie, we only support PCR == 0, actually.
>>
>> Okay, thanks for clarifying. Cedric suggested offline that I could
>> change "Unimplemented..." to "Invalid...". Are there any other changes
>> you would like?
>>
>
> No that's fine with me.
>
>> > So, this patch looks good for me. I'm just not sure about what is
>> > causing the build break with patchew though...
>>
>> I can't reproduce the failure here either.
>>
>
> Same here... :-\

I've got it. I wasn't careful about the PPC64 guards, so when you're
building for non-ppc64 targets it blows up. v2 incoming.