Re: [Qemu-devel] [RFC for-2.13 05/12] target/ppc: Remove fallback 64k pagesize information

[David Gibson]

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

signature.asc
Description: PGP signature