Re: [Qemu-devel] [PATCH 3/6] target/ppc: SDR1 is a hypervisor resource

[David Gibson]

On Thu, Feb 23, 2017 at 03:32:04PM +1100, Suraj Jitindar Singh wrote:
> On Thu, 2017-02-23 at 13:09 +1100, David Gibson wrote:
> > At present the SDR1 register - the base of the system's hashed page
> > table
> > (HPT) - is represented as an SPR with supervisor read and write
> > permission.
> > However, on CPUs which have a hypervisor mode, the SDR1 is a
> > hypervisor
> > only resource.  Change the permission checking on the SPR to reflect
> > this.
> > 
> > Now that this is done, we don't need to check for an external HPT
> > executing
> > mtsdr1: an external HPT only applies when we're emulating the
> > behaviour of
> > a hypervisor, rather than modelling the CPU's hypervisor mode
> > internally,
> > so if we're permitted to execute mtsdr1, we don't have an external
> > HPT.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  target/ppc/misc_helper.c    |  8 +++-----
> >  target/ppc/translate_init.c | 20 ++++++++++++++++----
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> > index ab432ba..fa573dd 100644
> > --- a/target/ppc/misc_helper.c
> > +++ b/target/ppc/misc_helper.c
> > @@ -82,11 +82,9 @@ void helper_store_sdr1(CPUPPCState *env,
> > target_ulong val)
> >  {
> >      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> >  
> > -    if (!env->external_htab) {
> > -        if (env->spr[SPR_SDR1] != val) {
> > -            ppc_store_sdr1(env, val);
> > -            tlb_flush(CPU(cpu));
> > -        }
> 
> It may have been the case we didn't have to check this before anyway...
> Oh well

Actually I think we did.  At least with a TCG (and maybe KVM PR)
guest, because the SDR1 was previously treated as a supervisor
resource, the guest could have issued an mtsdr1, changing the size of
the HPT.  We still would have read from the external_htab, but if the
size was changed the guest could have triggered accesses beyond the
end of it, crashing qemu.

With this check in place the behaviour of mtsdr1 was wrong (it was a
no-op instead of causing a privlieged instruction exception), but not
dangerous.

>
> > +    if (env->spr[SPR_SDR1] != val) {
> > +        ppc_store_sdr1(env, val);
> > +        tlb_flush(CPU(cpu));
> >      }
> >  }
> >  
> > diff --git a/target/ppc/translate_init.c
> > b/target/ppc/translate_init.c
> > index a1405e9..c92435d 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -740,10 +740,22 @@ static void gen_spr_ne_601 (CPUPPCState *env)
> >                   &spr_read_decr, &spr_write_decr,
> >                   0x00000000);
> >      /* Memory management */
> > -    spr_register(env, SPR_SDR1, "SDR1",
> > -                 SPR_NOACCESS, SPR_NOACCESS,
> > -                 &spr_read_generic, &spr_write_sdr1,
> > -                 0x00000000);
> > +#ifndef CONFIG_USER_ONLY
> > +    if (env->has_hv_mode) {
> > +        /* SDR1 is a hypervisor resource on CPUs which have a
> > +         * hypervisor mode */
> > +        spr_register_hv(env, SPR_SDR1, "SDR1",
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        SPR_NOACCESS, SPR_NOACCESS,
> > +                        &spr_read_generic, &spr_write_sdr1,
> > +                        0x00000000);
> > +    } else {
> > +        spr_register(env, SPR_SDR1, "SDR1",
> > +                     SPR_NOACCESS, SPR_NOACCESS,
> > +                     &spr_read_generic, &spr_write_sdr1,
> > +                     0x00000000);
> > +    }
> > +#endif
> >  }
> >  
> >  /* BATs 0-3 */
> 
> Reviewed-by: Suraj Jitindar Singh <address@hidden>
> 

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

signature.asc
Description: PGP signature