On Wed, Mar 28, 2018 at 10:01:38AM +0200, Greg Kurz wrote: > On Wed, 28 Mar 2018 11:32:04 +1100 > David Gibson 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 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 > > > > > > > > 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 > > > > --- > > > > 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) */ > > > > } > > > > > > -- 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