On Fri, Jan 13, 2017 at 05:28:15PM +1100, Suraj Jitindar Singh wrote:
> The SDR1 registers was used to store the location of the hash page table.
>
> This register no longer exists on POWER9 processors, so don't create it.
>
> We now store the hash page table location in the process table entry.
>
> We now check if the SDR1 register exists before printing its value when
> displaying register debug info.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsi...@gmail.com>
> ---
> target/ppc/kvm.c | 10 +++++++++-
> target/ppc/mmu-hash64.c | 15 ++++++++++++++-
> target/ppc/translate.c | 6 ++++--
> target/ppc/translate_init.c | 16 +++++++++++++---
> 4 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9c4834c..6016930 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -930,7 +930,15 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>
> sregs.pvr = env->spr[SPR_PVR];
>
> - sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> + switch (env->mmu_model) {
> + case POWERPC_MMU_3_00:
> + if (env->external_patbe)
> + sregs.u.s.sdr1 = env->external_patbe->patbe0;
I take it from this that KVM is expecting the address of the guest HPT
in the SDR1 slot of sregs, even on POWER9? Rather than using a new
ONE_REG entry for the POWER9 data.
Note that this slot was already a hack for PR KVM - we are putting a
userspace address in there, which is clearly not a real SDR1 value.
For HV KVM, I think this value is ignored entirely, since in that case
KVM allocates / controls the guest HPT, not qemu.
> + break;
> + default:
> + sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> + break;
> + }
So.. maybe we should take this opportunity to clean this path up and
special case all "external HPT" cases to pass the relevant value to PR
KVM without abusing the env->spr[SPR_SDR1] field.
>
> /* Sync SLB */
> #ifdef TARGET_PPC64
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index fe7da18..b9d4f4e 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -291,7 +291,20 @@ void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong
> value,
> CPUPPCState *env = &cpu->env;
> target_ulong htabsize = value & SDR_64_HTABSIZE;
>
> - env->spr[SPR_SDR1] = value;
> + switch (env->mmu_model) {
> + case POWERPC_MMU_3_00:
> + /*
> + * Technically P9 doesn't have a SDR1, the hash table address should
> be
> + * stored in the partition table entry instead.
> + */
> + if (env->external_patbe)
> + env->external_patbe->patbe0 = value;
This definitely doesn't belong in a function called ..._set_sdr1().
Either rename this function or (probably better), set the partition
table entry from a new function and make the decision in the caller.
> + break;
> + default:
> + env->spr[SPR_SDR1] = value;
> + break;
> + }
> +
> if (htabsize > 28) {
> error_setg(errp,
> "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 1212180..521aed3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6922,9 +6922,11 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf,
> case POWERPC_MMU_2_06a:
> case POWERPC_MMU_2_07:
> case POWERPC_MMU_2_07a:
> + case POWERPC_MMU_3_00:
> #endif
> - cpu_fprintf(f, " SDR1 " TARGET_FMT_lx " DAR " TARGET_FMT_lx
> - " DSISR " TARGET_FMT_lx "\n", env->spr[SPR_SDR1],
> + if (env->spr_cb[SPR_SDR1].name)
> + cpu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
> + cpu_fprintf(f, " DAR " TARGET_FMT_lx " DSISR " TARGET_FMT_lx "\n",
> env->spr[SPR_DAR], env->spr[SPR_DSISR]);
> break;
> case POWERPC_MMU_BOOKE206:
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index a1994d3..c771ba3 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -32,6 +32,7 @@
> #include "qapi/visitor.h"
> #include "hw/qdev-properties.h"
> #include "hw/ppc/ppc.h"
> +#include "mmu.h"
>
> //#define PPC_DUMP_CPU
> //#define PPC_DEBUG_SPR
> @@ -722,8 +723,8 @@ static void gen_spr_generic (CPUPPCState *env)
> 0x00000000);
> }
>
> -/* SPR common to all non-embedded PowerPC, including 601 */
> -static void gen_spr_ne_601 (CPUPPCState *env)
> +/* SPR common to all non-embedded PowerPC, including POWER9 */
> +static void gen_spr_ne_power9 (CPUPPCState *env)
> {
> /* Exception processing */
> spr_register_kvm(env, SPR_DSISR, "DSISR",
> @@ -739,6 +740,12 @@ static void gen_spr_ne_601 (CPUPPCState *env)
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_decr, &spr_write_decr,
> 0x00000000);
> +}
> +
> +/* SPR common to all non-embedded PowerPC, including 601 */
> +static void gen_spr_ne_601 (CPUPPCState *env)
> +{
> + gen_spr_ne_power9(env);
> /* Memory management */
> spr_register(env, SPR_SDR1, "SDR1",
> SPR_NOACCESS, SPR_NOACCESS,
> @@ -8222,7 +8229,6 @@ static void gen_spr_power8_rpr(CPUPPCState *env)
>
> static void init_proc_book3s_64(CPUPPCState *env, int version)
> {
> - gen_spr_ne_601(env);
> gen_tbl(env);
> gen_spr_book3s_altivec(env);
> gen_spr_book3s_pmu_sup(env);
> @@ -8280,6 +8286,10 @@ static void init_proc_book3s_64(CPUPPCState *env, int
> version)
> gen_spr_power8_book4(env);
> gen_spr_power8_rpr(env);
> }
> + if (version >= BOOK3S_CPU_POWER9)
> + gen_spr_ne_power9(env);
> + else
> + gen_spr_ne_601(env);
> if (version < BOOK3S_CPU_POWER8) {
> gen_spr_book3s_dbg(env);
> } else {
--
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
