Subject: Re: [PATCH] powerpc/64: Simplify adaptation to new ISA v3.00 HPTE format

From: "Aneesh Kumar K.V" <[email-masked]>
Date: Fri, 11 Nov 2016 17:59:33 +0530

Paul Mackerras <[email-masked]> writes:

> This changes the way that we support the new ISA v3.00 HPTE format.
> Instead of adapting everything that uses HPTE values to handle either
> the old format or the new format, depending on which CPU we are on,
> we now convert explicitly between old and new formats if necessary
> in the low-level routines that actually access HPTEs in memory.
> This limits the amount of code that needs to know about the new
> format and makes the conversions explicit.  This is OK because the
> old format contains all the information that is in the new format.
>
> This also fixes operation under a hypervisor, because the H_ENTER
> hypercall (and other hypercalls that deal with HPTEs) will continue
> to require the HPTE value to be supplied in the old format.  At
> present the kernel will not boot in HPT mode on POWER9 under a
> hypervisor.
>
> This fixes and partially reverts commit 50de596de8be
> ("powerpc/mm/hash: Add support for Power9 Hash", 2016-04-29).

This is better than the patch I sent. Do you think we need
s/new_v/3_0_v/ ?

Reviewed-by: Aneesh Kumar K.V <[email-masked]> 

>
> Fixes: 50de596de8be
> Signed-off-by: Paul Mackerras <[email-masked]>
> ---
>  arch/powerpc/include/asm/book3s/64/mmu-hash.h | 47 ++++++++++++++++++++++-----
>  arch/powerpc/mm/hash_native_64.c              | 30 +++++++++++++----
>  arch/powerpc/platforms/ps3/htab.c             |  2 +-
>  arch/powerpc/platforms/pseries/lpar.c         |  2 +-
>  4 files changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index e407af2..2e6a823 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -70,7 +70,9 @@
>
>  #define HPTE_V_SSIZE_SHIFT	62
>  #define HPTE_V_AVPN_SHIFT	7
> +#define HPTE_V_COMMON_BITS	ASM_CONST(0x000fffffffffffff)
>  #define HPTE_V_AVPN		ASM_CONST(0x3fffffffffffff80)
> +#define HPTE_V_AVPN_3_0		ASM_CONST(0x000fffffffffff80)
>  #define HPTE_V_AVPN_VAL(x)	(((x) & HPTE_V_AVPN) >> HPTE_V_AVPN_SHIFT)
>  #define HPTE_V_COMPARE(x,y)	(!(((x) ^ (y)) & 0xffffffffffffff80UL))
>  #define HPTE_V_BOLTED		ASM_CONST(0x0000000000000010)
> @@ -80,14 +82,16 @@
>  #define HPTE_V_VALID		ASM_CONST(0x0000000000000001)
>
>  /*
> - * ISA 3.0 have a different HPTE format.
> + * ISA 3.0 has a different HPTE format.
>   */
>  #define HPTE_R_3_0_SSIZE_SHIFT	58
> +#define HPTE_R_3_0_SSIZE_MASK	(3ull << HPTE_R_3_0_SSIZE_SHIFT)
>  #define HPTE_R_PP0		ASM_CONST(0x8000000000000000)
>  #define HPTE_R_TS		ASM_CONST(0x4000000000000000)
>  #define HPTE_R_KEY_HI		ASM_CONST(0x3000000000000000)
>  #define HPTE_R_RPN_SHIFT	12
>  #define HPTE_R_RPN		ASM_CONST(0x0ffffffffffff000)
> +#define HPTE_R_RPN_3_0		ASM_CONST(0x01fffffffffff000)
>  #define HPTE_R_PP		ASM_CONST(0x0000000000000003)
>  #define HPTE_R_PPP		ASM_CONST(0x8000000000000003)
>  #define HPTE_R_N		ASM_CONST(0x0000000000000004)
> @@ -316,12 +320,43 @@ static inline unsigned long hpte_encode_avpn(unsigned long vpn, int psize,
>  	 */
>  	v = (vpn >> (23 - VPN_SHIFT)) & ~(mmu_psize_defs[psize].avpnm);
>  	v <<= HPTE_V_AVPN_SHIFT;
> -	if (!cpu_has_feature(CPU_FTR_ARCH_300))
> -		v |= ((unsigned long) ssize) << HPTE_V_SSIZE_SHIFT;
> +	v |= ((unsigned long) ssize) << HPTE_V_SSIZE_SHIFT;
>  	return v;
>  }
>
>  /*
> + * ISA v3.0 defines a new HPTE format, which differs from the old
> + * format in having smaller AVPN and ARPN fields, and the B field
> + * in the second dword instead of the first.
> + */
> +static inline unsigned long hpte_old_to_new_v(unsigned long v)

How about

static inline unsigned long hpte_old_to_3_0_v(unsigned long v)

> +{
> +	/* trim AVPN, drop B */
> +	return v & HPTE_V_COMMON_BITS;
> +}
> +
> +static inline unsigned long hpte_old_to_new_r(unsigned long v, unsigned long r)
> +{
> +	/* move B field from 1st to 2nd dword, trim ARPN */
> +	return (r & ~HPTE_R_3_0_SSIZE_MASK) |
> +		(((v) >> HPTE_V_SSIZE_SHIFT) << HPTE_R_3_0_SSIZE_SHIFT);
> +}
> +
> +static inline unsigned long hpte_new_to_old_v(unsigned long v, unsigned long r)
> +{
> +	/* insert B field */
> +	return (v & HPTE_V_COMMON_BITS) |
> +		((r & HPTE_R_3_0_SSIZE_MASK) <<
> +		 (HPTE_V_SSIZE_SHIFT - HPTE_R_3_0_SSIZE_SHIFT));
> +}
> +
> +static inline unsigned long hpte_new_to_old_r(unsigned long r)
> +{
> +	/* clear out B field */
> +	return r & ~HPTE_R_3_0_SSIZE_MASK;
> +}
> +
> +/*
>   * This function sets the AVPN and L fields of the HPTE  appropriately
>   * using the base page size and actual page size.
>   */
> @@ -341,12 +376,8 @@ static inline unsigned long hpte_encode_v(unsigned long vpn, int base_psize,
>   * aligned for the requested page size
>   */
>  static inline unsigned long hpte_encode_r(unsigned long pa, int base_psize,
> -					  int actual_psize, int ssize)
> +					  int actual_psize)
>  {
> -
> -	if (cpu_has_feature(CPU_FTR_ARCH_300))
> -		pa |= ((unsigned long) ssize) << HPTE_R_3_0_SSIZE_SHIFT;
> -
>  	/* A 4K page needs no special encoding */
>  	if (actual_psize == MMU_PAGE_4K)
>  		return pa & HPTE_R_RPN;
> diff --git a/arch/powerpc/mm/hash_native_64.c b/arch/powerpc/mm/hash_native_64.c
> index 83ddc0e..ad9fd52 100644
> --- a/arch/powerpc/mm/hash_native_64.c
> +++ b/arch/powerpc/mm/hash_native_64.c
> @@ -221,13 +221,18 @@ static long native_hpte_insert(unsigned long hpte_group, unsigned long vpn,
>  		return -1;
>
>  	hpte_v = hpte_encode_v(vpn, psize, apsize, ssize) | vflags | HPTE_V_VALID;
> -	hpte_r = hpte_encode_r(pa, psize, apsize, ssize) | rflags;
> +	hpte_r = hpte_encode_r(pa, psize, apsize) | rflags;
>
>  	if (!(vflags & HPTE_V_BOLTED)) {
>  		DBG_LOW(" i=%x hpte_v=%016lx, hpte_r=%016lx\n",
>  			i, hpte_v, hpte_r);
>  	}
>
> +	if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> +		hpte_r = hpte_old_to_new_r(hpte_v, hpte_r);
> +		hpte_v = hpte_old_to_new_v(hpte_v);
> +	}
> +
>  	hptep->r = cpu_to_be64(hpte_r);
>  	/* Guarantee the second dword is visible before the valid bit */
>  	eieio();
> @@ -295,6 +300,8 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
>  		vpn, want_v & HPTE_V_AVPN, slot, newpp);
>
>  	hpte_v = be64_to_cpu(hptep->v);
> +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> +		hpte_v = hpte_new_to_old_v(hpte_v, be64_to_cpu(hptep->r));
>  	/*
>  	 * We need to invalidate the TLB always because hpte_remove doesn't do
>  	 * a tlb invalidate. If a hash bucket gets full, we "evict" a more/less
> @@ -309,6 +316,8 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
>  		native_lock_hpte(hptep);
>  		/* recheck with locks held */
>  		hpte_v = be64_to_cpu(hptep->v);
> +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> +			hpte_v = hpte_new_to_old_v(hpte_v, be64_to_cpu(hptep->r));
>  		if (unlikely(!HPTE_V_COMPARE(hpte_v, want_v) ||
>  			     !(hpte_v & HPTE_V_VALID))) {
>  			ret = -1;
> @@ -350,6 +359,8 @@ static long native_hpte_find(unsigned long vpn, int psize, int ssize)
>  	for (i = 0; i < HPTES_PER_GROUP; i++) {
>  		hptep = htab_address + slot;
>  		hpte_v = be64_to_cpu(hptep->v);
> +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> +			hpte_v = hpte_new_to_old_v(hpte_v, be64_to_cpu(hptep->r));
>
>  		if (HPTE_V_COMPARE(hpte_v, want_v) && (hpte_v & HPTE_V_VALID))
>  			/* HPTE matches */
> @@ -409,6 +420,8 @@ static void native_hpte_invalidate(unsigned long slot, unsigned long vpn,
>  	want_v = hpte_encode_avpn(vpn, bpsize, ssize);
>  	native_lock_hpte(hptep);
>  	hpte_v = be64_to_cpu(hptep->v);
> +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> +		hpte_v = hpte_new_to_old_v(hpte_v, be64_to_cpu(hptep->r));
>
>  	/*
>  	 * We need to invalidate the TLB always because hpte_remove doesn't do
> @@ -467,6 +480,8 @@ static void native_hugepage_invalidate(unsigned long vsid,
>  		want_v = hpte_encode_avpn(vpn, psize, ssize);
>  		native_lock_hpte(hptep);
>  		hpte_v = be64_to_cpu(hptep->v);
> +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> +			hpte_v = hpte_new_to_old_v(hpte_v, be64_to_cpu(hptep->r));
>
>  		/* Even if we miss, we need to invalidate the TLB */
>  		if (!HPTE_V_COMPARE(hpte_v, want_v) || !(hpte_v & HPTE_V_VALID))
> @@ -504,6 +519,10 @@ static void hpte_decode(struct hash_pte *hpte, unsigned long slot,
>  	/* Look at the 8 bit LP value */
>  	unsigned int lp = (hpte_r >> LP_SHIFT) & ((1 << LP_BITS) - 1);
>
> +	if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> +		hpte_v = hpte_new_to_old_v(hpte_v, hpte_r);
> +		hpte_r = hpte_new_to_old_r(hpte_r);
> +	}
>  	if (!(hpte_v & HPTE_V_LARGE)) {
>  		size   = MMU_PAGE_4K;
>  		a_size = MMU_PAGE_4K;
> @@ -512,11 +531,7 @@ static void hpte_decode(struct hash_pte *hpte, unsigned long slot,
>  		a_size = hpte_page_sizes[lp] >> 4;
>  	}
>  	/* This works for all page sizes, and for 256M and 1T segments */
> -	if (cpu_has_feature(CPU_FTR_ARCH_300))
> -		*ssize = hpte_r >> HPTE_R_3_0_SSIZE_SHIFT;
> -	else
> -		*ssize = hpte_v >> HPTE_V_SSIZE_SHIFT;
> -
> +	*ssize = hpte_v >> HPTE_V_SSIZE_SHIFT;
>  	shift = mmu_psize_defs[size].shift;
>
>  	avpn = (HPTE_V_AVPN_VAL(hpte_v) & ~mmu_psize_defs[size].avpnm);
> @@ -639,6 +654,9 @@ static void native_flush_hash_range(unsigned long number, int local)
>  			want_v = hpte_encode_avpn(vpn, psize, ssize);
>  			native_lock_hpte(hptep);
>  			hpte_v = be64_to_cpu(hptep->v);
> +			if (cpu_has_feature(CPU_FTR_ARCH_300))
> +				hpte_v = hpte_new_to_old_v(hpte_v,
> +						be64_to_cpu(hptep->r));
>  			if (!HPTE_V_COMPARE(hpte_v, want_v) ||
>  			    !(hpte_v & HPTE_V_VALID))
>  				native_unlock_hpte(hptep);
> diff --git a/arch/powerpc/platforms/ps3/htab.c b/arch/powerpc/platforms/ps3/htab.c
> index cb3c503..cc2b281 100644
> --- a/arch/powerpc/platforms/ps3/htab.c
> +++ b/arch/powerpc/platforms/ps3/htab.c
> @@ -63,7 +63,7 @@ static long ps3_hpte_insert(unsigned long hpte_group, unsigned long vpn,
>  	vflags &= ~HPTE_V_SECONDARY;
>
>  	hpte_v = hpte_encode_v(vpn, psize, apsize, ssize) | vflags | HPTE_V_VALID;
> -	hpte_r = hpte_encode_r(ps3_mm_phys_to_lpar(pa), psize, apsize, ssize) | rflags;
> +	hpte_r = hpte_encode_r(ps3_mm_phys_to_lpar(pa), psize, apsize) | rflags;
>
>  	spin_lock_irqsave(&ps3_htab_lock, flags);
>
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index aa35245..f2c98f6 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -145,7 +145,7 @@ static long pSeries_lpar_hpte_insert(unsigned long hpte_group,
>  			 hpte_group, vpn,  pa, rflags, vflags, psize);
>
>  	hpte_v = hpte_encode_v(vpn, psize, apsize, ssize) | vflags | HPTE_V_VALID;
> -	hpte_r = hpte_encode_r(pa, psize, apsize, ssize) | rflags;
> +	hpte_r = hpte_encode_r(pa, psize, apsize) | rflags;
>
>  	if (!(vflags & HPTE_V_BOLTED))
>  		pr_devel(" hpte_v=%016lx, hpte_r=%016lx\n", hpte_v, hpte_r);
> -- 
> 2.10.1