| Message ID | 20170223020936.29220-4-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <david@gibson.dropbear.id.au> > --- > 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 > + 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 <sjitindarsingh@gmail.com>
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 <david@gibson.dropbear.id.au> > > --- > > 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 <sjitindarsingh@gmail.com> >
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)); - } + 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 */
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 <david@gibson.dropbear.id.au> --- target/ppc/misc_helper.c | 8 +++----- target/ppc/translate_init.c | 20 ++++++++++++++++---- 2 files changed, 19 insertions(+), 9 deletions(-)