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

[Greg Kurz <groug@kaod.org>]

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) */
> > >  }  
> >   
>