| Message ID | 20180327043741.7705-6-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Assorted cpu cleanups (esp. hash64 MMU) | expand |
On 03/27/2018 06:37 AM, David Gibson wrote: > CPU definitions for cpus with the 64-bit hash MMU can include a table of > available pagesizes. If this isn't supplied ppc_cpu_instance_init() will > fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. > > However, it turns out all the cpus which support 64K pages already include > an explicit table of page sizes, so there's no point to the fallback table > including 64k pages. > > That removes the only place which tests POWERPC_MMU_64K, so we can remove > it. Which in turn allows some logic to be removed from > kvm_fixup_page_sizes(). > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > target/ppc/cpu-qom.h | 4 ---- > target/ppc/kvm.c | 7 ------- > target/ppc/translate_init.c | 20 ++------------------ > 3 files changed, 2 insertions(+), 29 deletions(-) > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > index deaa46a14b..9bbb05cf62 100644 > --- a/target/ppc/cpu-qom.h > +++ b/target/ppc/cpu-qom.h > @@ -70,7 +70,6 @@ enum powerpc_mmu_t { > #define POWERPC_MMU_64 0x00010000 > #define POWERPC_MMU_1TSEG 0x00020000 > #define POWERPC_MMU_AMR 0x00040000 > -#define POWERPC_MMU_64K 0x00080000 > #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ > /* 64 bits PowerPC MMU */ > POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, > @@ -78,15 +77,12 @@ enum powerpc_mmu_t { > POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, > /* Architecture 2.06 variant */ > POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | 0x00000003, > /* Architecture 2.07 variant */ > POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | 0x00000004, > /* Architecture 3.00 variant */ > POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | POWERPC_MMU_V3 > | 0x00000005, > }; > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 79a436a384..6160356a4a 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > static bool has_smmu_info; > CPUPPCState *env = &cpu->env; > int iq, ik, jq, jk; > - bool has_64k_pages = false; > > /* We only handle page sizes for 64-bit server guests for now */ > if (!(env->mmu_model & POWERPC_MMU_64)) { > @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > ksps->enc[jk].page_shift)) { > continue; > } > - if (ksps->enc[jk].page_shift == 16) { > - has_64k_pages = true; > - } > qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; > qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; > if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { > @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { > env->mmu_model &= ~POWERPC_MMU_1TSEG; > } > - if (!has_64k_pages) { > - env->mmu_model &= ~POWERPC_MMU_64K; > - } > } > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > index 29bd6f3654..99be6fcd68 100644 > --- a/target/ppc/translate_init.c > +++ b/target/ppc/translate_init.c > @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) > env->sps = *pcc->sps; > } else if (env->mmu_model & POWERPC_MMU_64) { > /* Use default sets of page sizes. We don't support MPSS */ > - static const struct ppc_segment_page_sizes defsps_4k = { > + static const struct ppc_segment_page_sizes defsps = { > .sps = { > { .page_shift = 12, /* 4K */ > .slb_enc = 0, > @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) > }, > }, > }; > - static const struct ppc_segment_page_sizes defsps_64k = { > - .sps = { > - { .page_shift = 12, /* 4K */ > - .slb_enc = 0, > - .enc = { { .page_shift = 12, .pte_enc = 0 } } > - }, > - { .page_shift = 16, /* 64K */ > - .slb_enc = 0x110, > - .enc = { { .page_shift = 16, .pte_enc = 1 } } > - }, > - { .page_shift = 24, /* 16M */ > - .slb_enc = 0x100, > - .enc = { { .page_shift = 24, .pte_enc = 0 } } > - }, > - }, > - }; > - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; > + env->sps = defsps; > } > #endif /* defined(TARGET_PPC64) */ > } >
On Tue, 27 Mar 2018 15:37:34 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > CPU definitions for cpus with the 64-bit hash MMU can include a table of > available pagesizes. If this isn't supplied ppc_cpu_instance_init() will > fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. > > However, it turns out all the cpus which support 64K pages already include > an explicit table of page sizes, so there's no point to the fallback table > including 64k pages. > I was thinking that 64k pages came with POWER5+. At least, this is mentioned in several places: https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.performance/supported_page_sizes_processor_type.htm https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4a0f2524acc2c602cadd8e743be19d86f3a746b And we do support POWER5+ with TCG and KVM PR. Shouldn't we include an explicit table of pages sizes there as well ? > That removes the only place which tests POWERPC_MMU_64K, so we can remove > it. Which in turn allows some logic to be removed from > kvm_fixup_page_sizes(). > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > target/ppc/cpu-qom.h | 4 ---- > target/ppc/kvm.c | 7 ------- > target/ppc/translate_init.c | 20 ++------------------ > 3 files changed, 2 insertions(+), 29 deletions(-) > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > index deaa46a14b..9bbb05cf62 100644 > --- a/target/ppc/cpu-qom.h > +++ b/target/ppc/cpu-qom.h > @@ -70,7 +70,6 @@ enum powerpc_mmu_t { > #define POWERPC_MMU_64 0x00010000 > #define POWERPC_MMU_1TSEG 0x00020000 > #define POWERPC_MMU_AMR 0x00040000 > -#define POWERPC_MMU_64K 0x00080000 > #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ > /* 64 bits PowerPC MMU */ > POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, > @@ -78,15 +77,12 @@ enum powerpc_mmu_t { > POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, > /* Architecture 2.06 variant */ > POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | 0x00000003, > /* Architecture 2.07 variant */ > POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | 0x00000004, > /* Architecture 3.00 variant */ > POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > - | POWERPC_MMU_64K > | POWERPC_MMU_AMR | POWERPC_MMU_V3 > | 0x00000005, > }; > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 79a436a384..6160356a4a 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > static bool has_smmu_info; > CPUPPCState *env = &cpu->env; > int iq, ik, jq, jk; > - bool has_64k_pages = false; > > /* We only handle page sizes for 64-bit server guests for now */ > if (!(env->mmu_model & POWERPC_MMU_64)) { > @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > ksps->enc[jk].page_shift)) { > continue; > } > - if (ksps->enc[jk].page_shift == 16) { > - has_64k_pages = true; > - } > qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; > qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; > if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { > @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { > env->mmu_model &= ~POWERPC_MMU_1TSEG; > } > - if (!has_64k_pages) { > - env->mmu_model &= ~POWERPC_MMU_64K; > - } > } > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > index 29bd6f3654..99be6fcd68 100644 > --- a/target/ppc/translate_init.c > +++ b/target/ppc/translate_init.c > @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) > env->sps = *pcc->sps; > } else if (env->mmu_model & POWERPC_MMU_64) { > /* Use default sets of page sizes. We don't support MPSS */ > - static const struct ppc_segment_page_sizes defsps_4k = { > + static const struct ppc_segment_page_sizes defsps = { > .sps = { > { .page_shift = 12, /* 4K */ > .slb_enc = 0, > @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) > }, > }, > }; > - static const struct ppc_segment_page_sizes defsps_64k = { > - .sps = { > - { .page_shift = 12, /* 4K */ > - .slb_enc = 0, > - .enc = { { .page_shift = 12, .pte_enc = 0 } } > - }, > - { .page_shift = 16, /* 64K */ > - .slb_enc = 0x110, > - .enc = { { .page_shift = 16, .pte_enc = 1 } } > - }, > - { .page_shift = 24, /* 16M */ > - .slb_enc = 0x100, > - .enc = { { .page_shift = 24, .pte_enc = 0 } } > - }, > - }, > - }; > - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; > + env->sps = defsps; > } > #endif /* defined(TARGET_PPC64) */ > }
On Tue, Mar 27, 2018 at 03:54:55PM +0200, Greg Kurz wrote: > On Tue, 27 Mar 2018 15:37:34 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > CPU definitions for cpus with the 64-bit hash MMU can include a table of > > available pagesizes. If this isn't supplied ppc_cpu_instance_init() will > > fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. > > > > However, it turns out all the cpus which support 64K pages already include > > an explicit table of page sizes, so there's no point to the fallback table > > including 64k pages. > > > > I was thinking that 64k pages came with POWER5+. At least, this is mentioned > in several places: > > https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.performance/supported_page_sizes_processor_type.htm > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4a0f2524acc2c602cadd8e743be19d86f3a746b Ok, I didn't know that. However, that was already wrong - we weren't setting the MMU_64K bit for POWER5+. > And we do support POWER5+ with TCG and KVM PR. Well, theoretically. I doubt it's been tested in years, and I strongly suspect it won't actually work. > Shouldn't we include an explicit > table of pages sizes there as well ? Yeah, but I think it makes more sense to fix that later. Or, more likely, not, since no-one actually cares about POWER5. > > > That removes the only place which tests POWERPC_MMU_64K, so we can remove > > it. Which in turn allows some logic to be removed from > > kvm_fixup_page_sizes(). > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > target/ppc/cpu-qom.h | 4 ---- > > target/ppc/kvm.c | 7 ------- > > target/ppc/translate_init.c | 20 ++------------------ > > 3 files changed, 2 insertions(+), 29 deletions(-) > > > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > > index deaa46a14b..9bbb05cf62 100644 > > --- a/target/ppc/cpu-qom.h > > +++ b/target/ppc/cpu-qom.h > > @@ -70,7 +70,6 @@ enum powerpc_mmu_t { > > #define POWERPC_MMU_64 0x00010000 > > #define POWERPC_MMU_1TSEG 0x00020000 > > #define POWERPC_MMU_AMR 0x00040000 > > -#define POWERPC_MMU_64K 0x00080000 > > #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ > > /* 64 bits PowerPC MMU */ > > POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, > > @@ -78,15 +77,12 @@ enum powerpc_mmu_t { > > POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, > > /* Architecture 2.06 variant */ > > POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > - | POWERPC_MMU_64K > > | POWERPC_MMU_AMR | 0x00000003, > > /* Architecture 2.07 variant */ > > POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > - | POWERPC_MMU_64K > > | POWERPC_MMU_AMR | 0x00000004, > > /* Architecture 3.00 variant */ > > POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > - | POWERPC_MMU_64K > > | POWERPC_MMU_AMR | POWERPC_MMU_V3 > > | 0x00000005, > > }; > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index 79a436a384..6160356a4a 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > static bool has_smmu_info; > > CPUPPCState *env = &cpu->env; > > int iq, ik, jq, jk; > > - bool has_64k_pages = false; > > > > /* We only handle page sizes for 64-bit server guests for now */ > > if (!(env->mmu_model & POWERPC_MMU_64)) { > > @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > ksps->enc[jk].page_shift)) { > > continue; > > } > > - if (ksps->enc[jk].page_shift == 16) { > > - has_64k_pages = true; > > - } > > qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; > > qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; > > if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { > > @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { > > env->mmu_model &= ~POWERPC_MMU_1TSEG; > > } > > - if (!has_64k_pages) { > > - env->mmu_model &= ~POWERPC_MMU_64K; > > - } > > } > > > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > index 29bd6f3654..99be6fcd68 100644 > > --- a/target/ppc/translate_init.c > > +++ b/target/ppc/translate_init.c > > @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) > > env->sps = *pcc->sps; > > } else if (env->mmu_model & POWERPC_MMU_64) { > > /* Use default sets of page sizes. We don't support MPSS */ > > - static const struct ppc_segment_page_sizes defsps_4k = { > > + static const struct ppc_segment_page_sizes defsps = { > > .sps = { > > { .page_shift = 12, /* 4K */ > > .slb_enc = 0, > > @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) > > }, > > }, > > }; > > - static const struct ppc_segment_page_sizes defsps_64k = { > > - .sps = { > > - { .page_shift = 12, /* 4K */ > > - .slb_enc = 0, > > - .enc = { { .page_shift = 12, .pte_enc = 0 } } > > - }, > > - { .page_shift = 16, /* 64K */ > > - .slb_enc = 0x110, > > - .enc = { { .page_shift = 16, .pte_enc = 1 } } > > - }, > > - { .page_shift = 24, /* 16M */ > > - .slb_enc = 0x100, > > - .enc = { { .page_shift = 24, .pte_enc = 0 } } > > - }, > > - }, > > - }; > > - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; > > + env->sps = defsps; > > } > > #endif /* defined(TARGET_PPC64) */ > > } >
On Wed, 28 Mar 2018 11:32:04 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Mar 27, 2018 at 03:54:55PM +0200, Greg Kurz wrote: > > On Tue, 27 Mar 2018 15:37:34 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > CPU definitions for cpus with the 64-bit hash MMU can include a table of > > > available pagesizes. If this isn't supplied ppc_cpu_instance_init() will > > > fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. > > > > > > However, it turns out all the cpus which support 64K pages already include > > > an explicit table of page sizes, so there's no point to the fallback table > > > including 64k pages. > > > > > > > I was thinking that 64k pages came with POWER5+. At least, this is mentioned > > in several places: > > > > https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.performance/supported_page_sizes_processor_type.htm > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4a0f2524acc2c602cadd8e743be19d86f3a746b > > Ok, I didn't know that. However, that was already wrong - we weren't > setting the MMU_64K bit for POWER5+. > Yes, I just happened to realize that while reviewing this patch. Hence the remark :) > > And we do support POWER5+ with TCG and KVM PR. > > Well, theoretically. I doubt it's been tested in years, and I > strongly suspect it won't actually work. > For the records, I could successfully boot a rhel67 guest on a POWER8 host with: -machine accel=kvm,kvm-type=PR,vsmt=1 -cpu power5+ but it fails with TCG. The guest kernel oopses at some point because of an illegal instruction and panics later on. > > Shouldn't we include an explicit > > table of pages sizes there as well ? > > Yeah, but I think it makes more sense to fix that later. Or, more > likely, not, since no-one actually cares about POWER5. > You're probably right. Anyway, this patch is the way to go, so: Reviewed-by: Greg Kurz <groug@kaod.org> > > > > > That removes the only place which tests POWERPC_MMU_64K, so we can remove > > > it. Which in turn allows some logic to be removed from > > > kvm_fixup_page_sizes(). > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > target/ppc/cpu-qom.h | 4 ---- > > > target/ppc/kvm.c | 7 ------- > > > target/ppc/translate_init.c | 20 ++------------------ > > > 3 files changed, 2 insertions(+), 29 deletions(-) > > > > > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > > > index deaa46a14b..9bbb05cf62 100644 > > > --- a/target/ppc/cpu-qom.h > > > +++ b/target/ppc/cpu-qom.h > > > @@ -70,7 +70,6 @@ enum powerpc_mmu_t { > > > #define POWERPC_MMU_64 0x00010000 > > > #define POWERPC_MMU_1TSEG 0x00020000 > > > #define POWERPC_MMU_AMR 0x00040000 > > > -#define POWERPC_MMU_64K 0x00080000 > > > #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ > > > /* 64 bits PowerPC MMU */ > > > POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, > > > @@ -78,15 +77,12 @@ enum powerpc_mmu_t { > > > POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, > > > /* Architecture 2.06 variant */ > > > POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > - | POWERPC_MMU_64K > > > | POWERPC_MMU_AMR | 0x00000003, > > > /* Architecture 2.07 variant */ > > > POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > - | POWERPC_MMU_64K > > > | POWERPC_MMU_AMR | 0x00000004, > > > /* Architecture 3.00 variant */ > > > POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > - | POWERPC_MMU_64K > > > | POWERPC_MMU_AMR | POWERPC_MMU_V3 > > > | 0x00000005, > > > }; > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > index 79a436a384..6160356a4a 100644 > > > --- a/target/ppc/kvm.c > > > +++ b/target/ppc/kvm.c > > > @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > static bool has_smmu_info; > > > CPUPPCState *env = &cpu->env; > > > int iq, ik, jq, jk; > > > - bool has_64k_pages = false; > > > > > > /* We only handle page sizes for 64-bit server guests for now */ > > > if (!(env->mmu_model & POWERPC_MMU_64)) { > > > @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > ksps->enc[jk].page_shift)) { > > > continue; > > > } > > > - if (ksps->enc[jk].page_shift == 16) { > > > - has_64k_pages = true; > > > - } > > > qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; > > > qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; > > > if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { > > > @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { > > > env->mmu_model &= ~POWERPC_MMU_1TSEG; > > > } > > > - if (!has_64k_pages) { > > > - env->mmu_model &= ~POWERPC_MMU_64K; > > > - } > > > } > > > > > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > index 29bd6f3654..99be6fcd68 100644 > > > --- a/target/ppc/translate_init.c > > > +++ b/target/ppc/translate_init.c > > > @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) > > > env->sps = *pcc->sps; > > > } else if (env->mmu_model & POWERPC_MMU_64) { > > > /* Use default sets of page sizes. We don't support MPSS */ > > > - static const struct ppc_segment_page_sizes defsps_4k = { > > > + static const struct ppc_segment_page_sizes defsps = { > > > .sps = { > > > { .page_shift = 12, /* 4K */ > > > .slb_enc = 0, > > > @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) > > > }, > > > }, > > > }; > > > - static const struct ppc_segment_page_sizes defsps_64k = { > > > - .sps = { > > > - { .page_shift = 12, /* 4K */ > > > - .slb_enc = 0, > > > - .enc = { { .page_shift = 12, .pte_enc = 0 } } > > > - }, > > > - { .page_shift = 16, /* 64K */ > > > - .slb_enc = 0x110, > > > - .enc = { { .page_shift = 16, .pte_enc = 1 } } > > > - }, > > > - { .page_shift = 24, /* 16M */ > > > - .slb_enc = 0x100, > > > - .enc = { { .page_shift = 24, .pte_enc = 0 } } > > > - }, > > > - }, > > > - }; > > > - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; > > > + env->sps = defsps; > > > } > > > #endif /* defined(TARGET_PPC64) */ > > > } > > >
On Wed, Mar 28, 2018 at 10:01:38AM +0200, Greg Kurz wrote: > On Wed, 28 Mar 2018 11:32:04 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Tue, Mar 27, 2018 at 03:54:55PM +0200, Greg Kurz wrote: > > > On Tue, 27 Mar 2018 15:37:34 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > CPU definitions for cpus with the 64-bit hash MMU can include a table of > > > > available pagesizes. If this isn't supplied ppc_cpu_instance_init() will > > > > fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. > > > > > > > > However, it turns out all the cpus which support 64K pages already include > > > > an explicit table of page sizes, so there's no point to the fallback table > > > > including 64k pages. > > > > > > > > > > I was thinking that 64k pages came with POWER5+. At least, this is mentioned > > > in several places: > > > > > > https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.performance/supported_page_sizes_processor_type.htm > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4a0f2524acc2c602cadd8e743be19d86f3a746b > > > > Ok, I didn't know that. However, that was already wrong - we weren't > > setting the MMU_64K bit for POWER5+. > > > > Yes, I just happened to realize that while reviewing this patch. Hence the > remark :) > > > > And we do support POWER5+ with TCG and KVM PR. > > > > Well, theoretically. I doubt it's been tested in years, and I > > strongly suspect it won't actually work. > > > > For the records, I could successfully boot a rhel67 guest on a POWER8 host > with: > > -machine accel=kvm,kvm-type=PR,vsmt=1 -cpu power5+ Yeah, under KVM (at least HV) I'm pretty sure it will effectively act like the host CPU, even if you try to specify something else. > but it fails with TCG. The guest kernel oopses at some point because > of an illegal instruction and panics later on. Right I'm pretty sure any guest distro that's even remotely modern will be compiler to use instructions that weren't available on power5+. > > > Shouldn't we include an explicit > > > table of pages sizes there as well ? > > > > Yeah, but I think it makes more sense to fix that later. Or, more > > likely, not, since no-one actually cares about POWER5. > > > > You're probably right. > > Anyway, this patch is the way to go, so: > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > > > > > That removes the only place which tests POWERPC_MMU_64K, so we can remove > > > > it. Which in turn allows some logic to be removed from > > > > kvm_fixup_page_sizes(). > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > --- > > > > target/ppc/cpu-qom.h | 4 ---- > > > > target/ppc/kvm.c | 7 ------- > > > > target/ppc/translate_init.c | 20 ++------------------ > > > > 3 files changed, 2 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > > > > index deaa46a14b..9bbb05cf62 100644 > > > > --- a/target/ppc/cpu-qom.h > > > > +++ b/target/ppc/cpu-qom.h > > > > @@ -70,7 +70,6 @@ enum powerpc_mmu_t { > > > > #define POWERPC_MMU_64 0x00010000 > > > > #define POWERPC_MMU_1TSEG 0x00020000 > > > > #define POWERPC_MMU_AMR 0x00040000 > > > > -#define POWERPC_MMU_64K 0x00080000 > > > > #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ > > > > /* 64 bits PowerPC MMU */ > > > > POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, > > > > @@ -78,15 +77,12 @@ enum powerpc_mmu_t { > > > > POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, > > > > /* Architecture 2.06 variant */ > > > > POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > > - | POWERPC_MMU_64K > > > > | POWERPC_MMU_AMR | 0x00000003, > > > > /* Architecture 2.07 variant */ > > > > POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > > - | POWERPC_MMU_64K > > > > | POWERPC_MMU_AMR | 0x00000004, > > > > /* Architecture 3.00 variant */ > > > > POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG > > > > - | POWERPC_MMU_64K > > > > | POWERPC_MMU_AMR | POWERPC_MMU_V3 > > > > | 0x00000005, > > > > }; > > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > > index 79a436a384..6160356a4a 100644 > > > > --- a/target/ppc/kvm.c > > > > +++ b/target/ppc/kvm.c > > > > @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > > static bool has_smmu_info; > > > > CPUPPCState *env = &cpu->env; > > > > int iq, ik, jq, jk; > > > > - bool has_64k_pages = false; > > > > > > > > /* We only handle page sizes for 64-bit server guests for now */ > > > > if (!(env->mmu_model & POWERPC_MMU_64)) { > > > > @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > > ksps->enc[jk].page_shift)) { > > > > continue; > > > > } > > > > - if (ksps->enc[jk].page_shift == 16) { > > > > - has_64k_pages = true; > > > > - } > > > > qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; > > > > qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; > > > > if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { > > > > @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) > > > > if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { > > > > env->mmu_model &= ~POWERPC_MMU_1TSEG; > > > > } > > > > - if (!has_64k_pages) { > > > > - env->mmu_model &= ~POWERPC_MMU_64K; > > > > - } > > > > } > > > > > > > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > > index 29bd6f3654..99be6fcd68 100644 > > > > --- a/target/ppc/translate_init.c > > > > +++ b/target/ppc/translate_init.c > > > > @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) > > > > env->sps = *pcc->sps; > > > > } else if (env->mmu_model & POWERPC_MMU_64) { > > > > /* Use default sets of page sizes. We don't support MPSS */ > > > > - static const struct ppc_segment_page_sizes defsps_4k = { > > > > + static const struct ppc_segment_page_sizes defsps = { > > > > .sps = { > > > > { .page_shift = 12, /* 4K */ > > > > .slb_enc = 0, > > > > @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) > > > > }, > > > > }, > > > > }; > > > > - static const struct ppc_segment_page_sizes defsps_64k = { > > > > - .sps = { > > > > - { .page_shift = 12, /* 4K */ > > > > - .slb_enc = 0, > > > > - .enc = { { .page_shift = 12, .pte_enc = 0 } } > > > > - }, > > > > - { .page_shift = 16, /* 64K */ > > > > - .slb_enc = 0x110, > > > > - .enc = { { .page_shift = 16, .pte_enc = 1 } } > > > > - }, > > > > - { .page_shift = 24, /* 16M */ > > > > - .slb_enc = 0x100, > > > > - .enc = { { .page_shift = 24, .pte_enc = 0 } } > > > > - }, > > > > - }, > > > > - }; > > > > - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; > > > > + env->sps = defsps; > > > > } > > > > #endif /* defined(TARGET_PPC64) */ > > > > } > > > > > >
diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h index deaa46a14b..9bbb05cf62 100644 --- a/target/ppc/cpu-qom.h +++ b/target/ppc/cpu-qom.h @@ -70,7 +70,6 @@ enum powerpc_mmu_t { #define POWERPC_MMU_64 0x00010000 #define POWERPC_MMU_1TSEG 0x00020000 #define POWERPC_MMU_AMR 0x00040000 -#define POWERPC_MMU_64K 0x00080000 #define POWERPC_MMU_V3 0x00100000 /* ISA V3.00 MMU Support */ /* 64 bits PowerPC MMU */ POWERPC_MMU_64B = POWERPC_MMU_64 | 0x00000001, @@ -78,15 +77,12 @@ enum powerpc_mmu_t { POWERPC_MMU_2_03 = POWERPC_MMU_64 | 0x00000002, /* Architecture 2.06 variant */ POWERPC_MMU_2_06 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG - | POWERPC_MMU_64K | POWERPC_MMU_AMR | 0x00000003, /* Architecture 2.07 variant */ POWERPC_MMU_2_07 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG - | POWERPC_MMU_64K | POWERPC_MMU_AMR | 0x00000004, /* Architecture 3.00 variant */ POWERPC_MMU_3_00 = POWERPC_MMU_64 | POWERPC_MMU_1TSEG - | POWERPC_MMU_64K | POWERPC_MMU_AMR | POWERPC_MMU_V3 | 0x00000005, }; diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index 79a436a384..6160356a4a 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -425,7 +425,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) static bool has_smmu_info; CPUPPCState *env = &cpu->env; int iq, ik, jq, jk; - bool has_64k_pages = false; /* We only handle page sizes for 64-bit server guests for now */ if (!(env->mmu_model & POWERPC_MMU_64)) { @@ -471,9 +470,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) ksps->enc[jk].page_shift)) { continue; } - if (ksps->enc[jk].page_shift == 16) { - has_64k_pages = true; - } qsps->enc[jq].page_shift = ksps->enc[jk].page_shift; qsps->enc[jq].pte_enc = ksps->enc[jk].pte_enc; if (++jq >= PPC_PAGE_SIZES_MAX_SZ) { @@ -488,9 +484,6 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu) if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) { env->mmu_model &= ~POWERPC_MMU_1TSEG; } - if (!has_64k_pages) { - env->mmu_model &= ~POWERPC_MMU_64K; - } } bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path) diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c index 29bd6f3654..99be6fcd68 100644 --- a/target/ppc/translate_init.c +++ b/target/ppc/translate_init.c @@ -10469,7 +10469,7 @@ static void ppc_cpu_instance_init(Object *obj) env->sps = *pcc->sps; } else if (env->mmu_model & POWERPC_MMU_64) { /* Use default sets of page sizes. We don't support MPSS */ - static const struct ppc_segment_page_sizes defsps_4k = { + static const struct ppc_segment_page_sizes defsps = { .sps = { { .page_shift = 12, /* 4K */ .slb_enc = 0, @@ -10481,23 +10481,7 @@ static void ppc_cpu_instance_init(Object *obj) }, }, }; - static const struct ppc_segment_page_sizes defsps_64k = { - .sps = { - { .page_shift = 12, /* 4K */ - .slb_enc = 0, - .enc = { { .page_shift = 12, .pte_enc = 0 } } - }, - { .page_shift = 16, /* 64K */ - .slb_enc = 0x110, - .enc = { { .page_shift = 16, .pte_enc = 1 } } - }, - { .page_shift = 24, /* 16M */ - .slb_enc = 0x100, - .enc = { { .page_shift = 24, .pte_enc = 0 } } - }, - }, - }; - env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k : defsps_4k; + env->sps = defsps; } #endif /* defined(TARGET_PPC64) */ }
CPU definitions for cpus with the 64-bit hash MMU can include a table of available pagesizes. If this isn't supplied ppc_cpu_instance_init() will fill it in a fallback table based on the POWERPC_MMU_64K bit in mmu_model. However, it turns out all the cpus which support 64K pages already include an explicit table of page sizes, so there's no point to the fallback table including 64k pages. That removes the only place which tests POWERPC_MMU_64K, so we can remove it. Which in turn allows some logic to be removed from kvm_fixup_page_sizes(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target/ppc/cpu-qom.h | 4 ---- target/ppc/kvm.c | 7 ------- target/ppc/translate_init.c | 20 ++------------------ 3 files changed, 2 insertions(+), 29 deletions(-)