Re: [Qemu-devel] [PATCH v3 06/24] target-ppc: Enable writes to user-privileged PMU registers

[Alexander Graf]

On 28.05.14 04:28, Alexey Kardashevskiy wrote:
> On 05/28/2014 10:03 AM, Alexander Graf wrote:
> > On 27.05.14 12:37, Alexey Kardashevskiy wrote:
> > > This adds spr_write_ureg() helper and uses it for UPMCx and MMCR0 SPRs.
> > >
> > > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > > ---
> > >    target-ppc/translate_init.c | 25 +++++++++++++++----------
> > >    1 file changed, 15 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > > index 88acf70..595fd3f 100644
> > > --- a/target-ppc/translate_init.c
> > > +++ b/target-ppc/translate_init.c
> > > @@ -175,6 +175,11 @@ static void spr_read_ureg (void *opaque, int gprn,
> > > int sprn)
> > >        gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
> > >    }
> > >    +static void spr_write_ureg(void *opaque, int gprn, int sprn)
> > > +{
> > > +    gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
> > > +}
> > > +
> > >    /* SPR common to all non-embedded PowerPC */
> > >    /* DECR */
> > >    #if !defined(CONFIG_USER_ONLY)
> > > @@ -7379,28 +7384,28 @@ static void gen_spr_book3s_pmu(CPUPPCState *env)
> > >                     &spr_read_generic, SPR_NOACCESS,
> > >                     0x00000000);
> > >        spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > +                 &spr_read_ureg, &spr_write_ureg,
> > > +                 &spr_read_ureg, &spr_write_ureg,
> > Are you sure about this? The 970MP spec says "Performance Monitor Registers
> > (For Reading)". In general, I don't think we would ever really have an
> > spr_write_ureg user.
> Uffff. It is worse than that. They were read-only for user space till 2.07,
> now they might be write-enabled too because of EBB. Do I really have to
> implement exceptions on accessing them as described in "6.2.11 Hypervisor
> Facility Status and Control Register" (2.07)? It is getting too much just
> to get H_SET_MODE(AIL) accepted, no? :-)

Just treat them as not write enabled and leave the EBB magic for later. 
Indicate that it's missing and what needs to be done with a comment :).


Alex