Re: [Qemu-devel] [PATCH 03/17] ppc: Add a bunch of hypervisor SPRs to Book3s

[David Gibson <david@gibson.dropbear.id.au>]

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


On Mon, Mar 14, 2016 at 08:14:59PM +0100, Thomas Huth wrote:
> On 14.03.2016 17:56, Cédric Le Goater wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > We don't give them a KVM reg number to most of the registers yet as no
> > current KVM version supports HV mode. For DAWR and DAWRX, the KVM reg
> > number is needed since this register can be set by the guest via the
> > H_SET_MODE hypercall.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > [clg: squashed in patch 'ppc: Add KVM numbers to some P8 SPRs' and
> >       changed the commit log with a proposal of Thomas Huth ]
> > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> > ---
> >  target-ppc/translate_init.c | 140 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 137 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 6a11b41206e5..43c6e524a6bc 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -1105,6 +1105,11 @@ static void gen_spr_amr (CPUPPCState *env)
> >                       SPR_NOACCESS, SPR_NOACCESS,
> >                       &spr_read_generic, &spr_write_generic,
> >                       KVM_REG_PPC_UAMOR, 0);
> > +    spr_register_hv(env, SPR_AMOR, "AMOR",
> > +                    SPR_NOACCESS, SPR_NOACCESS,
> > +                    SPR_NOACCESS, SPR_NOACCESS,
> > +                    &spr_read_generic, &spr_write_generic,
> > +                    0);
> >  #endif /* !CONFIG_USER_ONLY */
> >  }
> >  #endif /* TARGET_PPC64 */
> > @@ -7491,6 +7496,20 @@ static void gen_spr_book3s_dbg(CPUPPCState *env)
> >                       KVM_REG_PPC_DABRX, 0x00000000);
> >  }
> >  
> > +static void gen_spr_book3s_207_dbg(CPUPPCState *env)
> > +{
> > +    spr_register_kvm_hv(env, SPR_DAWR, "DAWR",
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        &spr_read_generic, &spr_write_generic,
> > +                        KVM_REG_PPC_DAWR, 0x00000000);
> > +    spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        &spr_read_generic, &spr_write_generic,
> > +                        KVM_REG_PPC_DAWRX, 0x00000000);
> > +}
> > +
> >  static void gen_spr_970_dbg(CPUPPCState *env)
> >  {
> >      /* Breakpoints */
> > @@ -7683,15 +7702,116 @@ static void gen_spr_power5p_lpar(CPUPPCState *env)
> >      spr_register_kvm(env, SPR_LPCR, "LPCR",
> >                       SPR_NOACCESS, SPR_NOACCESS,
> >                       &spr_read_generic, &spr_write_generic,
> > -                     KVM_REG_PPC_LPCR, 0x00000000);
> > +                     KVM_REG_PPC_LPCR, LPCR_LPES0 | LPCR_LPES1);
> 
> Could we please postpone that hunk to a later, separate patch (after
> QEMU 2.6 has been released)? It looks like it could maybe cause some
> trouble with some emulated boards (e.g. there is some code in
> target-ppc/excp_helper.c for example - which is currently disabled, but
> I'm not sure whether there are other spots like this somewhere else).

I think this whole patch needs to wait until after 2.6, I'm not seeing
a good rationale for squeezing it into 2.6 at this stage.

> >  }
> >  
> > +#if !defined(CONFIG_USER_ONLY)
> > +static void spr_write_hmer(DisasContext *ctx, int sprn, int gprn)
> > +{
> > +    TCGv hmer = tcg_temp_new();
> > +
> > +    gen_load_spr(hmer, sprn);
> > +    tcg_gen_and_tl(hmer, cpu_gpr[gprn], hmer);
> > +    gen_store_spr(sprn, hmer);
> > +    spr_store_dump_spr(sprn);
> > +    tcg_temp_free(hmer);
> > +}
> > +#endif
> > +
> >  static void gen_spr_book3s_ids(CPUPPCState *env)
> >  {
> > +    /* FIXME: Will need to deal with thread vs core only SPRs */
> > +
> >      /* Processor identification */
> > -    spr_register(env, SPR_PIR, "PIR",
> > +    spr_register_hv(env, SPR_PIR, "PIR",
> >                   SPR_NOACCESS, SPR_NOACCESS,
> > -                 &spr_read_generic, &spr_write_pir,
> > +                 SPR_NOACCESS, SPR_NOACCESS,
> > +                 &spr_read_generic, NULL,
> > +                 0x00000000);
> 
> What does the NULL mean here? I haven't seen any other spr_register*()
> calls yet that pass NULL as parameter for a handler. Should that maybe
> rather be a SPR_NOACCESS instead?
> 
>  Thomas
> 

-- 
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: 819 bytes --]