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

[Greg Kurz]

On Mon, 4 Jun 2018 17:59:45 +1000
David Gibson <address@hidden> wrote:

> On Thu, May 31, 2018 at 01:27:33PM +0200, Greg Kurz 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).  
> 
> That's really not true.  pcr_mask is relevant at the hardware level,
> not just for pseries.  It's the mask of bits that are implemented in
> the PCR for this CPU.
> 

Oops... dumb me stroke again... So indeed, the pcr_mask should be applied
here, but the (value != 0) check makes sense anyway since TCG currently
ignores the PCR.

> pcr_supported isn't really relevant here regardless of whether we're
> talking about pseries or pnv.  It's basically just a kind of weird
> encoding of which compat modes are supported by the cpu.
> 
> > 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.
> > 
> > So, this patch looks good for me. I'm just not sure about what is
> > causing the build break with patchew though...
> >   
> > > 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);
> > > >  }
> > > >  
> > > >     
> > > 
> > >   
> >   
>

pgpK2p3smdHTY.pgp
Description: OpenPGP digital signature