| Message ID | 1484288903-18807-10-git-send-email-sjitindarsingh@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 <sjitindarsingh@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 {
On Wed, 2017-02-01 at 15:16 +1100, David Gibson wrote: > 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 <sjitindarsingh@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. We don't support KVM_PR or PNV machine type for POWER9 yet. I will update this patch to remove the idea of a SDR1 which will be fine for pseries machines. Support for a partition table register will need to be added to enable PNV for power9 emulation, but that will come later. > > > > > + 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 {
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; + break; + default: + sregs.u.s.sdr1 = env->spr[SPR_SDR1]; + break; + } /* 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; + 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 {
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 <sjitindarsingh@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(-)