[PATCH] ppc: SDR1 fixes for ppc64

[Andreas Färber]

Adjust shifts and masks to 64-bit SDR1.

Cc: Alexander Graf 
Signed-off-by: Andreas Färber 
---
 arch/ppc/qemu/ofmem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index fd417c2..9c60841 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -61,7 +61,7 @@ get_hash_base( void )

asm volatile("mfsdr1 %0" : "=r" (sdr1) );

-	return (sdr1 & 0xffff0000);
+	return (sdr1 & ~0xffffUL);
 }

static inline unsigned long
@@ -71,7 +71,7 @@ get_hash_size( void )

asm volatile("mfsdr1 %0" : "=r" (sdr1) );

-	return ((sdr1 << 16) | 0x0000ffff) + 1;
+	return (((sdr1 & 0x1f) << 16) | 0x0000ffff) + 1;
 }

static inline unsigned long

[Segher Boessenkool]

> @@ -71,7 +71,7 @@ get_hash_size( void )
>
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return ((sdr1 << 16) | 0x0000ffff) + 1;
> return (((sdr1 & 0x1f) << 16) | 0x0000ffff) + 1;
>
> }

Shouldn't this actually be something like
  0x10000UL << (sdr1 & 0x1f)
?

Segher

[Andreas Färber]

Am 15.11.2010 um 23:32 schrieb Segher Boessenkool:
> > @@ -71,7 +71,7 @@ get_hash_size( void )
> >
> > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > return ((sdr1 << 16) | 0x0000ffff) + 1;
> > return (((sdr1 & 0x1f) << 16) | 0x0000ffff) + 1;
> >
> > }
>
> Shouldn't this actually be something like
>  0x10000UL << (sdr1 & 0x1f)
> ?

No, I don't think so. HTABSIZE (formerly HTABMASK) is a bit mask, not  
a number. Thus the +1.

You did spot an error though, thanks. The 16 should probably be 11 and  
therefore the 0xffff 0x7ff.

Andreas

[Segher Boessenkool]

> > > return ((sdr1 << 16) | 0x0000ffff) + 1;
> > > return (((sdr1 & 0x1f) << 16) | 0x0000ffff) + 1;
> > >
> > > }
> >
> > Shouldn't this actually be something like
> >  0x10000UL << (sdr1 & 0x1f)
> > ?
>
> No, I don't think so. HTABSIZE (formerly HTABMASK) is a bit mask, not
> a number. Thus the +1.

PowerISA v2.05 says:

"The HTABSIZE field in SDR1 contains an integer giving the number of bits
(in addition to the minimum of 11 bits) from the hash that are used in the
Page Table index."

It is like this on even the oldest 64-bit implementations (a 5-bit
integer, add 11; max is 28, so 11..39 bits used, 2**18..2**47 bytes htab
size).

In the 32-bit "classic" PEM, it's a bitmask indeed (9 bits, all zeroes
means 10 bits used, so 10..19 bits used, 2**16..2**25 bytes htab size).

Segher

[Segher Boessenkool]

> It is like this on even the oldest 64-bit implementations (a 5-bit
> integer, add 11; max is 28, so 11..39 bits used, 2**18..2**47 bytes htab
> size).

2**18..2**46, typo.  Or perhaps I fail at arithmetic :-P

Segher

[Andreas Färber]

Am 20.11.2010 um 23:30 schrieb Segher Boessenkool:
> > > > return ((sdr1 << 16) | 0x0000ffff) + 1;
> > > > return (((sdr1 & 0x1f) << 16) | 0x0000ffff) + 1;
> > > >
> > > > }
> > >
> > > Shouldn't this actually be something like
> > > 0x10000UL << (sdr1 & 0x1f)
> > > ?
> >
> > No, I don't think so. HTABSIZE (formerly HTABMASK) is a bit mask, not
> > a number. Thus the +1.
>
> PowerISA v2.05 says:
>
> "The HTABSIZE field in SDR1 contains an integer giving the number of  
> bits
> (in addition to the minimum of 11 bits) from the hash that are used  
> in the
> Page Table index."
>
> It is like this on even the oldest 64-bit implementations (a 5-bit
> integer, add 11; max is 28, so 11..39 bits used, 2**18..2**47 bytes  
> htab
> size).
>
> In the 32-bit "classic" PEM, it's a bitmask indeed (9 bits, all zeroes
> means 10 bits used, so 10..19 bits used, 2**16..2**25 bytes htab  
> size).

Yeah, spotted the difference in the meantime and working on the patch.  
I'm differentiating based on __powerpc64__ but that should probably be  
based on PVR instead? If so, then how?

Andreas

[RFC v2] ppc: SDR1 fixes

[Andreas Färber]

Clean up use of SDR1 fields.

IBM's "The Programming Environments for 32-Bit Microprocessors":
  The HTABMASK field in SDR1 contains a mask value that determines
  how many bits from the hash are used in the page table index. This mask
  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
  from the hash are used in the index; HTABORG must have this same number of
  low-order bits equal to 0.

IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
  The HTABSIZE field in SDR1 contains an integer value that determines
  how many bits from the hash are used in the page table index. This number
  must not exceed 28. HTABSIZE is used to generate a mask of the form
  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
  The 1-bits determine how many additional bits (beyond the minimum of 11)
  from the hash are used in the index. The HTABORG must have this same number
  of low-order bits equal to 0.

Adjust masks and shifts to 64-bit SDR1.

v2:
* Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
  Error spotted by Segher.

Cc: Alexander Graf 
Cc: Segher Boessenkool 
Signed-off-by: Andreas Färber 
---
 Note: This breaks 32-bit ppc at least.

arch/ppc/qemu/ofmem.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index fd417c2..6f2b86d 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -48,7 +48,8 @@ extern void setup_mmu( unsigned long code_base );
 #define OF_CODE_START	0xfff00000UL
 #define IO_BASE			0x80000000

-#define HASH_SIZE		(2 << 15)
+#define HASH_BITS		15
+#define HASH_SIZE		(2 << HASH_BITS)
 #define OFMEM_SIZE		(2 * 1024 * 1024)

#define	SEGR_USER		BIT(2)
@@ -57,21 +58,29 @@ extern void setup_mmu( unsigned long code_base );
 static inline unsigned long
 get_hash_base( void )
 {
-	unsigned long sdr1;
+    unsigned long sdr1;

-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
+    asm volatile("mfsdr1 %0" : "=r" (sdr1) );

-	return (sdr1 & 0xffff0000);
+#ifdef __powerpc64__
+    return (sdr1 & 0x3FFFFFFFFFFC0000UL);
+#else
+    return (sdr1 & 0xffff0000UL);
+#endif
 }

static inline unsigned long
 get_hash_size( void )
 {
-	unsigned long sdr1;
+    unsigned long sdr1;

-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
+    asm volatile("mfsdr1 %0" : "=r" (sdr1));

-	return ((sdr1 << 16) | 0x0000ffff) + 1;
+#ifdef __powerpc64__
+    return 1UL << (sdr1 & 0x1f);
+#else
+    return (((sdr1 & 0x1ff) << 10) | 0x3ff) + 1;
+#endif
 }

static inline unsigned long
@@ -389,14 +398,18 @@ setup_mmu( unsigned long ramsize )
    ofmem_t *ofmem;
    unsigned long sdr1, sr_base, msr;
    unsigned long hash_base;
-	unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
+	unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
    int i;

/* SDR1: Storage Description Register 1 */

hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
    memset((void *)hash_base, 0, HASH_SIZE);
-	sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
+#ifdef __powerpc64__
+	sdr1 = hash_base | HASH_BITS;
+#else
+	sdr1 = hash_base | ((HASH_SIZE - 1) >> 10);
+#endif
    asm volatile("mtsdr1 %0" :: "r" (sdr1) );

/* Segment Register */

[RFC v2] ppc: SDR1 fixes

[Segher Boessenkool]

> Signed-off-by: Andreas Färber

Acked-by: Segher Boessenkool

But one change please...
> +#ifdef __powerpc64__
> return 1UL << (sdr1 & 0x1f);
>
> +#else
> return (((sdr1 & 0x1ff) << 10) | 0x3ff) + 1;
>
> +#endif

((sdr1 & 0x1ff) + 1) << 10

looks a bit less silly :-)

I'm not sure you got the size calculations right, but let's assume
you tested it :-)

Segher

[RFC v2] ppc: SDR1 fixes

[Andreas Färber]

Am 21.11.2010 um 01:10 schrieb Segher Boessenkool:
> > Signed-off-by: Andreas Färber
>
> Acked-by: Segher Boessenkool
>
> But one change please...
> > +#ifdef __powerpc64__
> > return 1UL << (sdr1 & 0x1f);
> >
> > +#else
> > return (((sdr1 & 0x1ff) << 10) | 0x3ff) + 1;
> >
> > +#endif
>
> ((sdr1 & 0x1ff) + 1) << 10
>
> looks a bit less silly :-)
>
> I'm not sure you got the size calculations right, but let's assume
> you tested it :-)

I'm not sure either, since my testing - as stated - breaks ppc.  
Haven't debugged into ppc64 yet.

I was hoping someone spots an obvious error, because if my code were  
correct it would mean a bug hidden somewhere in QEMU... :-/

Andreas

[RFC v2] ppc: SDR1 fixes

[Andreas Färber]

Am 21.11.2010 um 01:24 schrieb Andreas Färber:
> Am 21.11.2010 um 01:10 schrieb Segher Boessenkool:
> > > Signed-off-by: Andreas Färber
> >
> > Acked-by: Segher Boessenkool
> >
> > But one change please...
> > > +#ifdef __powerpc64__
> > > return 1UL << (sdr1 & 0x1f);
> > >
> > > +#else
> > > return (((sdr1 & 0x1ff) << 10) | 0x3ff) + 1;
> > >
> > > +#endif
> >
> > ((sdr1 & 0x1ff) + 1) << 10
> >
> > looks a bit less silly :-)
> >
> > I'm not sure you got the size calculations right, but let's assume
> > you tested it :-)
>
> I'm not sure either, since my testing - as stated - breaks ppc.  
> Haven't debugged into ppc64 yet.
>
> I was hoping someone spots an obvious error, because if my code were  
> correct it would mean a bug hidden somewhere in QEMU... :-/

It turned out that on ppc my SDR1 shifts by 10 don't work despite the  
manual mentioning that number, with original 16 it works for whatever  
reason. (Alex, you don't happen to know why, do you? I peeked at  
target-ppc/helper.c without spotting something obvious.)

ppc64 needs the slbmte patch first for testing (0x700 otherwise).  
HASH_BITS likely should be 16 instead of 15 since 2 was shifted, not 1.

Andreas

[RFC v2] ppc: SDR1 fixes

[Segher Boessenkool]

> It turned out that on ppc my SDR1 shifts by 10 don't work despite the
> manual mentioning that number, with original 16 it works for whatever
> reason.

10 is the minimal number of bits of the hash used in indexing the
PTEGs.  Each PTEG is 64 bytes.  So if you're calculating the size
here, you get (the number of bits in the mask) + 10 + 6, as log2
of number of bytes.
> ppc64 needs the slbmte patch first for testing (0x700 otherwise).
> HASH_BITS likely should be 16 instead of 15 since 2 was shifted, not 1.

That sure looks confusing as well.  PTEGs on 64-bit are 128 bytes.

Segher

[PATCH v3] ppc: SDR1 fixes

[Andreas Färber]

Clean up use of SDR1 fields.

IBM's "The Programming Environments for 32-Bit Microprocessors":
  The HTABMASK field in SDR1 contains a mask value that determines
  how many bits from the hash are used in the page table index. This mask
  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
  from the hash are used in the index; HTABORG must have this same number of
  low-order bits equal to 0.

IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
  The HTABSIZE field in SDR1 contains an integer value that determines
  how many bits from the hash are used in the page table index. This number
  must not exceed 28. HTABSIZE is used to generate a mask of the form
  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
  The 1-bits determine how many additional bits (beyond the minimum of 11)
  from the hash are used in the index. The HTABORG must have this same number
  of low-order bits equal to 0.

Adjust alignment mask and hash size for ppc64.

Note that the page table does not yet scale with the memory size, and
the secondary hash is not yet used.

v3:
* Fix shifts. Thanks to Segher and Alex.
* Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
  so that it worked as expected. Avoid underflow, spotted by Alex.
  Make this fix available for legacy 64-bit support as well.
* Drop get_hash_size(). It was only used in hash_page_32() and caused
  unnecessary conversions between HTABMASK and size.
* Introduce {mfsdr1,mtsdr1} as suggested by Alex.
  Note that this changes the volatility for hash_page_64().
* Conditionalize 64-bit support, drop 32-bit support for ppc64.
* Adjust hash size for ppc64.

v2:
* Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
  Error spotted by Segher.

Cc: Alexander Graf 
Cc: Segher Boessenkool 
Signed-off-by: Andreas Färber 
---
 arch/ppc/qemu/ofmem.c        |   65 +++++++++++++++++++++++++++--------------
 include/arch/ppc/processor.h |   12 ++++++++
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index 72694b3..40a23ff 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
 #define OF_CODE_START	0xfff00000UL
 #define IO_BASE			0x80000000

-#define HASH_SIZE		(2 << 15)
+#ifdef __powerpc64__
+#define HASH_BITS		18
+#else
+#define HASH_BITS		15
+#endif
+#define HASH_SIZE		(2 << HASH_BITS)
 #define OFMEM_SIZE		(2 * 1024 * 1024)

#define	SEGR_USER		BIT(2)
@@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
 static inline unsigned long
 get_hash_base( void )
 {
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return (sdr1 & 0xffff0000);
-}
-
-static inline unsigned long
-get_hash_size( void )
-{
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return ((sdr1 << 16) | 0x0000ffff) + 1;
+#ifdef __powerpc64__
+    return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
+#else
+    return (mfsdr1() & 0xffff0000UL);
+#endif
 }

static inline unsigned long
@@ -228,6 +223,11 @@ ea_to_phys( ucell ea, ucell *mode )
    return phys;
 }

+#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
+#ifndef CONFIG_PPC_64BITSUPPORT
+#define hash_page_64 hash_page
+#endif
+
 static void
 hash_page_64( ucell ea, ucell phys, ucell mode )
 {
@@ -242,7 +242,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
    vsid = (ea >> 28) + SEGR_BASE;
    vsid_sh = 7;
    vsid_mask = 0x00003FFFFFFFFF80ULL;
-	asm ( "mfsdr1 %0" : "=r" (sdr) );
+	sdr = mfsdr1();
    sdr_sh = 18;
    sdr_mask = 0x3FF80;
    page_mask = 0x0FFFFFFF; // XXX correct?
@@ -294,6 +294,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
    asm volatile( "tlbie %0"  :: "r"(ea) );
 }

+#endif /* ppc64 */
+#ifndef __powerpc64__
+#ifndef CONFIG_PPC_64BITSUPPORT
+#define hash_page_32 hash_page
+#endif
+
 static void
 hash_page_32( ucell ea, ucell phys, ucell mode )
 {
@@ -307,7 +313,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )

hash1 = vsid;
    hash1 ^= (ea >> 12) & 0xffff;
-	hash1 &= (get_hash_size() - 1) >> 6;
+	hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;

pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
    upte = (unsigned long*)pp;
@@ -334,6 +340,8 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
    asm volatile( "tlbie %0"  :: "r"(ea) );
 }

+#ifdef CONFIG_PPC_64BITSUPPORT
+
 static int is_ppc64(void)
 {
    unsigned int pvr;
@@ -350,6 +358,9 @@ static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
    	hash_page_32(ea, phys, mode);
 }

+#endif /* CONFIG_PPC_64BITSUPPORT */
+#endif /* ppc */
+
 void
 dsi_exception( void )
 {
@@ -387,17 +398,27 @@ void
 setup_mmu( unsigned long ramsize )
 {
    ofmem_t *ofmem;
-	unsigned long sdr1, sr_base;
+	unsigned long sr_base;
    unsigned long hash_base;
-	unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
+	unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
    int i;

/* SDR1: Storage Description Register 1 */

hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
    memset((void *)hash_base, 0, HASH_SIZE);
-	sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
-	asm volatile("mtsdr1 %0" :: "r" (sdr1) );
+#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
+#ifdef CONFIG_PPC_64BITSUPPORT
+	if (is_ppc64())
+#endif
+		mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
+#ifdef CONFIG_PPC_64BITSUPPORT
+	else
+#endif
+#endif
+#ifndef __powerpc64__
+		mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
+#endif

/* Segment Register */

diff --git a/include/arch/ppc/processor.h b/include/arch/ppc/processor.h
index c7d5be6..329c5cd 100644
--- a/include/arch/ppc/processor.h
+++ b/include/arch/ppc/processor.h
@@ -425,6 +425,18 @@ static inline void mtmsr(unsigned long msr)
 #endif
 }

+static inline unsigned long mfsdr1(void)
+{
+    unsigned long sdr1;
+    asm volatile("mfsdr1 %0" : "=r" (sdr1));
+    return sdr1;
+}
+
+static inline void mtsdr1(unsigned long sdr1)
+{
+    asm volatile("mtsdr1 %0" :: "r" (sdr1));
+}
+
 #endif /* !__ASSEMBLER__ */

#endif   /* _H_PROCESSOR */

[PATCH v3] ppc: SDR1 fixes

[Alexander Graf]

On 21.11.2010, at 18:50, Andreas Färber wrote:
> Clean up use of SDR1 fields.
>
> IBM's "The Programming Environments for 32-Bit Microprocessors":
>  The HTABMASK field in SDR1 contains a mask value that determines
>  how many bits from the hash are used in the page table index. This mask
>  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
>  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
>  from the hash are used in the index; HTABORG must have this same number of
>  low-order bits equal to 0.
>
> IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
>  The HTABSIZE field in SDR1 contains an integer value that determines
>  how many bits from the hash are used in the page table index. This number
>  must not exceed 28. HTABSIZE is used to generate a mask of the form
>  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
>  The 1-bits determine how many additional bits (beyond the minimum of 11)
>  from the hash are used in the index. The HTABORG must have this same number
>  of low-order bits equal to 0.
>
> Adjust alignment mask and hash size for ppc64.
>
> Note that the page table does not yet scale with the memory size, and
> the secondary hash is not yet used.
>
> v3:
> Fix shifts. Thanks to Segher and Alex.
> Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
>
> so that it worked as expected. Avoid underflow, spotted by Alex.
>  Make this fix available for legacy 64-bit support as well.
> Drop get_hash_size(). It was only used in hash_page_32() and caused
>
> unnecessary conversions between HTABMASK and size.
> Introduce {mfsdr1,mtsdr1} as suggested by Alex.
>
> Note that this changes the volatility for hash_page_64().
> Conditionalize 64-bit support, drop 32-bit support for ppc64.
> Adjust hash size for ppc64.
>
> v2:
> Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
>
> Error spotted by Segher.
>
> Cc: Alexander Graf 
> Cc: Segher Boessenkool 
> Signed-off-by: Andreas Färber
>
> arch/ppc/qemu/ofmem.c        |   65 +++++++++++++++++++++++++++--------------
> include/arch/ppc/processor.h |   12 ++++++++
> 2 files changed, 55 insertions(+), 22 deletions(-)
>
> diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> index 72694b3..40a23ff 100644
> --- a/arch/ppc/qemu/ofmem.c
> +++ b/arch/ppc/qemu/ofmem.c
> @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
> #define OF_CODE_START	0xfff00000UL
> #define IO_BASE			0x80000000
>
> -#define HASH_SIZE		(2 << 15)
> +#ifdef __powerpc64__
> +#define HASH_BITS		18
> +#else
> +#define HASH_BITS		15
> +#endif
> +#define HASH_SIZE		(2 << HASH_BITS)
> #define OFMEM_SIZE		(2 * 1024 * 1024)
>
> #define	SEGR_USER		BIT(2)
> @@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
> static inline unsigned long
> get_hash_base( void )
> {
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return (sdr1 & 0xffff0000);
>
> -}
>
> -static inline unsigned long
> -get_hash_size( void )
> -{
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return ((sdr1 << 16) | 0x0000ffff) + 1;
>
> +#ifdef __powerpc64__
> return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
>
> +#else
> return (mfsdr1() & 0xffff0000UL);
>
> +#endif
> }
>
> static inline unsigned long
> @@ -228,6 +223,11 @@ ea_to_phys( ucell ea, ucell *mode )
>    return phys;
> }
>
> +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> +#ifndef CONFIG_PPC_64BITSUPPORT
> +#define hash_page_64 hash_page
> +#endif

I'm not sure why, but this doesn't feel good :). Too much preprocessor magic. Maybe explicitly call hash_page_64 or hash_page_32 depending on the outcome of if_ppc64() and not preprocess things?
> static void
> hash_page_64( ucell ea, ucell phys, ucell mode )
> {
> @@ -242,7 +242,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
>    vsid = (ea >> 28) + SEGR_BASE;
>    vsid_sh = 7;
>    vsid_mask = 0x00003FFFFFFFFF80ULL;
> asm ( "mfsdr1 %0" : "=r" (sdr) );
> sdr = mfsdr1();
> sdr_sh = 18;
> sdr_mask = 0x3FF80;
> page_mask = 0x0FFFFFFF; // XXX correct?
>
> @@ -294,6 +294,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
>    asm volatile( "tlbie %0"  :: "r"(ea) );
> }
>
> +#endif /* ppc64 */
> +#ifndef __powerpc64__
> +#ifndef CONFIG_PPC_64BITSUPPORT
> +#define hash_page_32 hash_page
> +#endif
>
> static void
> hash_page_32( ucell ea, ucell phys, ucell mode )
> {
> @@ -307,7 +313,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>
> hash1 = vsid;
>    hash1 ^= (ea >> 12) & 0xffff;
> hash1 &= (get_hash_size() - 1) >> 6;
>
> hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
>
> pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> upte = (unsigned long*)pp;
>
> @@ -334,6 +340,8 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>    asm volatile( "tlbie %0"  :: "r"(ea) );
> }
>
> +#ifdef CONFIG_PPC_64BITSUPPORT
>
> static int is_ppc64(void)
> {
>    unsigned int pvr;
> @@ -350,6 +358,9 @@ static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
>    	hash_page_32(ea, phys, mode);
> }
>
> +#endif /* CONFIG_PPC_64BITSUPPORT */
> +#endif /* ppc */
>
> void
> dsi_exception( void )
> {
> @@ -387,17 +398,27 @@ void
> setup_mmu( unsigned long ramsize )
> {
>    ofmem_t *ofmem;
> unsigned long sdr1, sr_base;
> unsigned long sr_base;
> unsigned long hash_base;
> unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
>
> unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
> int i;
>
> /* SDR1: Storage Description Register 1 */
>
> hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> memset((void *)hash_base, 0, HASH_SIZE);
> sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> asm volatile("mtsdr1 %0" :: "r" (sdr1) );
>
> +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> +#ifdef CONFIG_PPC_64BITSUPPORT
> if (is_ppc64())
>
> +#endif
> mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
>
> +#ifdef CONFIG_PPC_64BITSUPPORT
> else
>
> +#endif
> +#endif
> +#ifndef __powerpc64__
> mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
>
> +#endif

Wow, in this case I guess it'd be better to keep the code clean and simply make is_ppc64() always return 0 on !defined(CONFIG_PPC_64BITSUPPORT).

Alex

[PATCH v3] ppc: SDR1 fixes

[Andreas Färber]

Am 21.11.2010 um 20:07 schrieb Alexander Graf:
> On 21.11.2010, at 18:50, Andreas Färber wrote:
> > Clean up use of SDR1 fields.
> >
> > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > The HTABMASK field in SDR1 contains a mask value that determines
> > how many bits from the hash are used in the page table index. This  
> > mask
> > must be of the form 0b00...011...1; that is, a string of 0 bits  
> > followed by
> > a string of 1bits. The 1 bits determine how many additional bits  
> > (at least 10)
> > from the hash are used in the index; HTABORG must have this same  
> > number of
> > low-order bits equal to 0.
> >
> > IBM's "The Programming Environments Manual for 64-bit  
> > Microprocessors" 3.0:
> > The HTABSIZE field in SDR1 contains an integer value that determines
> > how many bits from the hash are used in the page table index. This  
> > number
> > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > 0b00...011...1; that is, a string of 0 bits followed by a string of  
> > 1-bits.
> > The 1-bits determine how many additional bits (beyond the minimum  
> > of 11)
> > from the hash are used in the index. The HTABORG must have this  
> > same number
> > of low-order bits equal to 0.
> >
> > Adjust alignment mask and hash size for ppc64.
> >
> > Note that the page table does not yet scale with the memory size, and
> > the secondary hash is not yet used.
> >
> > v3:
> > Fix shifts. Thanks to Segher and Alex.
> > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to
> >
> > be 0,
> > so that it worked as expected. Avoid underflow, spotted by Alex.
> > Make this fix available for legacy 64-bit support as well.
> > Drop get_hash_size(). It was only used in hash_page_32() and caused
> >
> > unnecessary conversions between HTABMASK and size.
> > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> >
> > Note that this changes the volatility for hash_page_64().
> > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > Adjust hash size for ppc64.
> >
> > v2:
> > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> >
> > Error spotted by Segher.
> >
> > Cc: Alexander Graf 
> > Cc: Segher Boessenkool 
> > Signed-off-by: Andreas Färber
> > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > index 72694b3..40a23ff 100644
> > --- a/arch/ppc/qemu/ofmem.c
> > +++ b/arch/ppc/qemu/ofmem.c
> > @@ -228,6 +223,11 @@ ea_to_phys( ucell ea, ucell *mode )
> >    return phys;
> > }
> >
> > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > +#ifndef CONFIG_PPC_64BITSUPPORT
> > +#define hash_page_64 hash_page
> > +#endif
>
> I'm not sure why, but this doesn't feel good :). Too much  
> preprocessor magic. Maybe explicitly call hash_page_64 or  
> hash_page_32 depending on the outcome of if_ppc64() and not  
> preprocess things?

Actually I am serious about getting rid of hash_page_32() in real  
ppc64 code. I have an idea to simplify this. On second thoughts the  
processor magic might be confusing when debugging.

Andreas

[PATCH v4] ppc: SDR1 fixes

[Andreas Färber]

Clean up use of SDR1 fields.

IBM's "The Programming Environments for 32-Bit Microprocessors":
  The HTABMASK field in SDR1 contains a mask value that determines
  how many bits from the hash are used in the page table index. This mask
  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
  from the hash are used in the index; HTABORG must have this same number of
  low-order bits equal to 0.

IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
  The HTABSIZE field in SDR1 contains an integer value that determines
  how many bits from the hash are used in the page table index. This number
  must not exceed 28. HTABSIZE is used to generate a mask of the form
  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
  The 1-bits determine how many additional bits (beyond the minimum of 11)
  from the hash are used in the index. The HTABORG must have this same number
  of low-order bits equal to 0.

Adjust alignment mask and hash size for ppc64.

Note that the page table does not yet scale with the memory size, and
the secondary hash is not yet used.

v4:
* Don't redefine hash_page_{32,64} but add TODO comments for renaming.
* Keep and simplify hash_page().

v3:
* Fix shifts. Thanks to Segher and Alex.
* Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
  so that it worked as expected. Avoid underflow, spotted by Alex.
  Make this fix available for legacy 64-bit support as well.
* Drop get_hash_size(). It was only used in hash_page_32() and caused
  unnecessary conversions between HTABMASK and size.
* Introduce {mfsdr1,mtsdr1} as suggested by Alex.
  Note that this changes the volatility for hash_page_64().
* Conditionalize 64-bit support, drop 32-bit support for ppc64.
* Adjust hash size for ppc64.

v2:
* Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
  Error spotted by Segher.

Cc: Alexander Graf 
Cc: Segher Boessenkool 
Signed-off-by: Andreas Färber 
---
 arch/ppc/qemu/ofmem.c        |   69 ++++++++++++++++++++++++++++-------------
 include/arch/ppc/processor.h |   12 +++++++
 2 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index 72694b3..6f3079a 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
 #define OF_CODE_START	0xfff00000UL
 #define IO_BASE			0x80000000

-#define HASH_SIZE		(2 << 15)
+#ifdef __powerpc64__
+#define HASH_BITS		18
+#else
+#define HASH_BITS		15
+#endif
+#define HASH_SIZE		(2 << HASH_BITS)
 #define OFMEM_SIZE		(2 * 1024 * 1024)

#define	SEGR_USER		BIT(2)
@@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
 static inline unsigned long
 get_hash_base( void )
 {
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return (sdr1 & 0xffff0000);
-}
-
-static inline unsigned long
-get_hash_size( void )
-{
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return ((sdr1 << 16) | 0x0000ffff) + 1;
+#ifdef __powerpc64__
+    return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
+#else
+    return (mfsdr1() & 0xffff0000UL);
+#endif
 }

static inline unsigned long
@@ -228,6 +223,8 @@ ea_to_phys( ucell ea, ucell *mode )
    return phys;
 }

+#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
+
 static void
 hash_page_64( ucell ea, ucell phys, ucell mode )
 {
@@ -242,7 +239,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
    vsid = (ea >> 28) + SEGR_BASE;
    vsid_sh = 7;
    vsid_mask = 0x00003FFFFFFFFF80ULL;
-	asm ( "mfsdr1 %0" : "=r" (sdr) );
+	sdr = mfsdr1();
    sdr_sh = 18;
    sdr_mask = 0x3FF80;
    page_mask = 0x0FFFFFFF; // XXX correct?
@@ -294,6 +291,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
    asm volatile( "tlbie %0"  :: "r"(ea) );
 }

+#endif /* ppc64 */
+#ifndef __powerpc64__
+/* XXX Turn this into an #else when legacy 64-bit support is gone
+ *     and rename hash_page{32,64} to hash_page.
+ */
+
 static void
 hash_page_32( ucell ea, ucell phys, ucell mode )
 {
@@ -307,7 +310,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )

hash1 = vsid;
    hash1 ^= (ea >> 12) & 0xffff;
-	hash1 &= (get_hash_size() - 1) >> 6;
+	hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;

pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
    upte = (unsigned long*)pp;
@@ -334,6 +337,9 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
    asm volatile( "tlbie %0"  :: "r"(ea) );
 }

+#endif /* ppc */
+#ifdef CONFIG_PPC_64BITSUPPORT
+
 static int is_ppc64(void)
 {
    unsigned int pvr;
@@ -342,12 +348,21 @@ static int is_ppc64(void)
    return ((pvr >= 0x330000) && (pvr < 0x70330000));
 }

+#endif /* CONFIG_PPC_64BITSUPPORT */
+
+/* XXX Drop this function when legacy 64-bit support is dropped. */
 static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
 {
+#ifdef __powerpc64__
+	hash_page_64(ea, phys, mode);
+#else
+#ifdef CONFIG_PPC_64BITSUPPORT
    if ( is_ppc64() )
    	hash_page_64(ea, phys, mode);
    else
+#endif
    	hash_page_32(ea, phys, mode);
+#endif
 }

void
@@ -387,17 +402,27 @@ void
 setup_mmu( unsigned long ramsize )
 {
    ofmem_t *ofmem;
-	unsigned long sdr1, sr_base;
+	unsigned long sr_base;
    unsigned long hash_base;
-	unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
+	unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
    int i;

/* SDR1: Storage Description Register 1 */

hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
    memset((void *)hash_base, 0, HASH_SIZE);
-	sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
-	asm volatile("mtsdr1 %0" :: "r" (sdr1) );
+#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
+#ifdef CONFIG_PPC_64BITSUPPORT
+	if (is_ppc64())
+#endif
+		mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
+#ifdef CONFIG_PPC_64BITSUPPORT
+	else
+#endif
+#endif
+#ifndef __powerpc64__
+		mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
+#endif

/* Segment Register */

diff --git a/include/arch/ppc/processor.h b/include/arch/ppc/processor.h
index c7d5be6..329c5cd 100644
--- a/include/arch/ppc/processor.h
+++ b/include/arch/ppc/processor.h
@@ -425,6 +425,18 @@ static inline void mtmsr(unsigned long msr)
 #endif
 }

+static inline unsigned long mfsdr1(void)
+{
+    unsigned long sdr1;
+    asm volatile("mfsdr1 %0" : "=r" (sdr1));
+    return sdr1;
+}
+
+static inline void mtsdr1(unsigned long sdr1)
+{
+    asm volatile("mtsdr1 %0" :: "r" (sdr1));
+}
+
 #endif /* !__ASSEMBLER__ */

#endif   /* _H_PROCESSOR */

[PATCH v4] ppc: SDR1 fixes

[Alexander Graf]

On 21.11.2010, at 21:08, Andreas Färber wrote:
> Clean up use of SDR1 fields.
>
> IBM's "The Programming Environments for 32-Bit Microprocessors":
>  The HTABMASK field in SDR1 contains a mask value that determines
>  how many bits from the hash are used in the page table index. This mask
>  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
>  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
>  from the hash are used in the index; HTABORG must have this same number of
>  low-order bits equal to 0.
>
> IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
>  The HTABSIZE field in SDR1 contains an integer value that determines
>  how many bits from the hash are used in the page table index. This number
>  must not exceed 28. HTABSIZE is used to generate a mask of the form
>  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
>  The 1-bits determine how many additional bits (beyond the minimum of 11)
>  from the hash are used in the index. The HTABORG must have this same number
>  of low-order bits equal to 0.
>
> Adjust alignment mask and hash size for ppc64.
>
> Note that the page table does not yet scale with the memory size, and
> the secondary hash is not yet used.
>
> v4:
> Don't redefine hash_page_{32,64} but add TODO comments for renaming.
> Keep and simplify hash_page().
>
> v3:
> Fix shifts. Thanks to Segher and Alex.
> Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
>
> so that it worked as expected. Avoid underflow, spotted by Alex.
>  Make this fix available for legacy 64-bit support as well.
> Drop get_hash_size(). It was only used in hash_page_32() and caused
>
> unnecessary conversions between HTABMASK and size.
> Introduce {mfsdr1,mtsdr1} as suggested by Alex.
>
> Note that this changes the volatility for hash_page_64().
> Conditionalize 64-bit support, drop 32-bit support for ppc64.
> Adjust hash size for ppc64.
>
> v2:
> Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
>
> Error spotted by Segher.
>
> Cc: Alexander Graf 
> Cc: Segher Boessenkool 
> Signed-off-by: Andreas Färber
>
> arch/ppc/qemu/ofmem.c        |   69 ++++++++++++++++++++++++++++-------------
> include/arch/ppc/processor.h |   12 +++++++
> 2 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> index 72694b3..6f3079a 100644
> --- a/arch/ppc/qemu/ofmem.c
> +++ b/arch/ppc/qemu/ofmem.c
> @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
> #define OF_CODE_START	0xfff00000UL
> #define IO_BASE			0x80000000
>
> -#define HASH_SIZE		(2 << 15)
> +#ifdef __powerpc64__
> +#define HASH_BITS		18
> +#else
> +#define HASH_BITS		15
> +#endif
> +#define HASH_SIZE		(2 << HASH_BITS)
> #define OFMEM_SIZE		(2 * 1024 * 1024)
>
> #define	SEGR_USER		BIT(2)
> @@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
> static inline unsigned long
> get_hash_base( void )
> {
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return (sdr1 & 0xffff0000);
>
> -}
>
> -static inline unsigned long
> -get_hash_size( void )
> -{
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return ((sdr1 << 16) | 0x0000ffff) + 1;
>
> +#ifdef __powerpc64__
> return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);

Just define these as costants and keep the actual code clean :).
> +#else
> return (mfsdr1() & 0xffff0000UL);
>
> +#endif
> }
>
> static inline unsigned long
> @@ -228,6 +223,8 @@ ea_to_phys( ucell ea, ucell *mode )
>    return phys;
> }
>
> +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
>
> static void
> hash_page_64( ucell ea, ucell phys, ucell mode )
> {
> @@ -242,7 +239,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
>    vsid = (ea >> 28) + SEGR_BASE;
>    vsid_sh = 7;
>    vsid_mask = 0x00003FFFFFFFFF80ULL;
> asm ( "mfsdr1 %0" : "=r" (sdr) );
> sdr = mfsdr1();
> sdr_sh = 18;
> sdr_mask = 0x3FF80;
> page_mask = 0x0FFFFFFF; // XXX correct?
>
> @@ -294,6 +291,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
>    asm volatile( "tlbie %0"  :: "r"(ea) );
> }
>
> +#endif /* ppc64 */
> +#ifndef __powerpc64__
> +/* XXX Turn this into an #else when legacy 64-bit support is gone
> and rename hash_page{32,64} to hash_page.

Just keep them around. They shouldn't hurt. The code will be optimized away by the compiler anyways if CONFIG_PPC_64BITSUPPORT is not set, as static functions that never get caled (if on a constant 0 is never get called) just don't get compiled in.
> */
>
> static void
> hash_page_32( ucell ea, ucell phys, ucell mode )
> {
> @@ -307,7 +310,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>
> hash1 = vsid;
>    hash1 ^= (ea >> 12) & 0xffff;
> hash1 &= (get_hash_size() - 1) >> 6;
>
> hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
>
> pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> upte = (unsigned long*)pp;
>
> @@ -334,6 +337,9 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>    asm volatile( "tlbie %0"  :: "r"(ea) );
> }
>
> +#endif /* ppc */
> +#ifdef CONFIG_PPC_64BITSUPPORT
>
> static int is_ppc64(void)
> {
>    unsigned int pvr;
> @@ -342,12 +348,21 @@ static int is_ppc64(void)
>    return ((pvr >= 0x330000) && (pvr < 0x70330000));
> }
>
> +#endif /* CONFIG_PPC_64BITSUPPORT */

static int is_ppc64(void)
{
#if defined(__powerpc64__)
    return 1;
#elif defined(CONFIG_PPC_64BITSUPPORT)
    unsigned int pvr = mfpvr();
    return ((pvr >= 0x330000) && (pvr < 0x70330000));
#else
    return 0;
#endif
}
> +/* XXX Drop this function when legacy 64-bit support is dropped. */
> static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
> {
> +#ifdef __powerpc64__
> hash_page_64(ea, phys, mode);
>
> +#else
> +#ifdef CONFIG_PPC_64BITSUPPORT
>    if ( is_ppc64() )
>    	hash_page_64(ea, phys, mode);
>    else
> +#endif
>    	hash_page_32(ea, phys, mode);
> +#endif

if (is_ppc64)
        hash_page_64(ea, phys, mode);
    else
        hash_page_32(ea, phys, mode);
> }
>
> void
> @@ -387,17 +402,27 @@ void
> setup_mmu( unsigned long ramsize )
> {
>    ofmem_t *ofmem;
> unsigned long sdr1, sr_base;
> unsigned long sr_base;
> unsigned long hash_base;
> unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
>
> unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
> int i;
>
> /* SDR1: Storage Description Register 1 */
>
> hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> memset((void *)hash_base, 0, HASH_SIZE);
> sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> asm volatile("mtsdr1 %0" :: "r" (sdr1) );
>
> +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> +#ifdef CONFIG_PPC_64BITSUPPORT
> if (is_ppc64())
>
> +#endif
> mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
>
> +#ifdef CONFIG_PPC_64BITSUPPORT
> else
>
> +#endif
> +#endif
> +#ifndef __powerpc64__
> mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
>
> +#endif

if (is_ppc64())
        mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
    else
        mtsdr1(hash_base | (HASH_SIZE - 1) >> 16, 0));

That's a lot easier to read, no?

Alex

[PATCH v4] ppc: SDR1 fixes

[Andreas Färber]

Am 21.11.2010 um 22:22 schrieb Alexander Graf:
> On 21.11.2010, at 21:08, Andreas Färber wrote:
> > Clean up use of SDR1 fields.
> >
> > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > The HTABMASK field in SDR1 contains a mask value that determines
> > how many bits from the hash are used in the page table index. This  
> > mask
> > must be of the form 0b00...011...1; that is, a string of 0 bits  
> > followed by
> > a string of 1bits. The 1 bits determine how many additional bits  
> > (at least 10)
> > from the hash are used in the index; HTABORG must have this same  
> > number of
> > low-order bits equal to 0.
> >
> > IBM's "The Programming Environments Manual for 64-bit  
> > Microprocessors" 3.0:
> > The HTABSIZE field in SDR1 contains an integer value that determines
> > how many bits from the hash are used in the page table index. This  
> > number
> > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > 0b00...011...1; that is, a string of 0 bits followed by a string of  
> > 1-bits.
> > The 1-bits determine how many additional bits (beyond the minimum  
> > of 11)
> > from the hash are used in the index. The HTABORG must have this  
> > same number
> > of low-order bits equal to 0.
> >
> > Adjust alignment mask and hash size for ppc64.
> >
> > Note that the page table does not yet scale with the memory size, and
> > the secondary hash is not yet used.
> >
> > v4:
> > Don't redefine hash_page_{32,64} but add TODO comments for
> >
> > renaming.
> > Keep and simplify hash_page().
> >
> > v3:
> > Fix shifts. Thanks to Segher and Alex.
> > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to
> >
> > be 0,
> > so that it worked as expected. Avoid underflow, spotted by Alex.
> > Make this fix available for legacy 64-bit support as well.
> > Drop get_hash_size(). It was only used in hash_page_32() and caused
> >
> > unnecessary conversions between HTABMASK and size.
> > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> >
> > Note that this changes the volatility for hash_page_64().
> > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > Adjust hash size for ppc64.
> >
> > v2:
> > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> >
> > Error spotted by Segher.
> >
> > Cc: Alexander Graf 
> > Cc: Segher Boessenkool 
> > Signed-off-by: Andreas Färber
> >
> > arch/ppc/qemu/ofmem.c        |   69 +++++++++++++++++++++++++++ 
> > +-------------
> > include/arch/ppc/processor.h |   12 +++++++
> > 2 files changed, 59 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > index 72694b3..6f3079a 100644
> > --- a/arch/ppc/qemu/ofmem.c
> > +++ b/arch/ppc/qemu/ofmem.c
> > @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
> > #define OF_CODE_START	0xfff00000UL
> > #define IO_BASE			0x80000000
> >
> > -#define HASH_SIZE		(2 << 15)
> > +#ifdef __powerpc64__
> > +#define HASH_BITS		18
> > +#else
> > +#define HASH_BITS		15
> > +#endif
> > +#define HASH_SIZE		(2 << HASH_BITS)
> > #define OFMEM_SIZE		(2 * 1024 * 1024)
> >
> > #define	SEGR_USER		BIT(2)
> > @@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
> > static inline unsigned long
> > get_hash_base( void )
> > {
> > unsigned long sdr1;
> > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > return (sdr1 & 0xffff0000);
> >
> > -}
> >
> > -static inline unsigned long
> > -get_hash_size( void )
> > -{
> > unsigned long sdr1;
> > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > return ((sdr1 << 16) | 0x0000ffff) + 1;
> >
> > +#ifdef __powerpc64__
> > return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
>
> Just define these as costants and keep the actual code clean :).

OK.
> > +#else
> > return (mfsdr1() & 0xffff0000UL);
> >
> > +#endif
> > }
> >
> > static inline unsigned long
> > @@ -228,6 +223,8 @@ ea_to_phys( ucell ea, ucell *mode )
> >    return phys;
> > }
> >
> > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> >
> > static void
> > hash_page_64( ucell ea, ucell phys, ucell mode )
> > {
> > @@ -242,7 +239,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
> >    vsid = (ea >> 28) + SEGR_BASE;
> >    vsid_sh = 7;
> >    vsid_mask = 0x00003FFFFFFFFF80ULL;
> > asm ( "mfsdr1 %0" : "=r" (sdr) );
> > sdr = mfsdr1();
> > sdr_sh = 18;
> > sdr_mask = 0x3FF80;
> > page_mask = 0x0FFFFFFF; // XXX correct?
> >
> > @@ -294,6 +291,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
> >    asm volatile( "tlbie %0"  :: "r"(ea) );
> > }
> >
> > +#endif /* ppc64 */
> > +#ifndef __powerpc64__
> > +/* XXX Turn this into an #else when legacy 64-bit support is gone
> > and rename hash_page{32,64} to hash_page.
>
> Just keep them around. They shouldn't hurt. The code will be  
> optimized away by the compiler anyways if CONFIG_PPC_64BITSUPPORT is  
> not set, as static functions that never get caled (if on a constant  
> 0 is never get called) just don't get compiled in.

They do hurt in that not calling them leads to an error.
> > */
> >
> > static void
> > hash_page_32( ucell ea, ucell phys, ucell mode )
> > {
> > @@ -307,7 +310,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
> >
> > hash1 = vsid;
> >    hash1 ^= (ea >> 12) & 0xffff;
> > hash1 &= (get_hash_size() - 1) >> 6;
> >
> > hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
> >
> > pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> > upte = (unsigned long*)pp;
> >
> > @@ -334,6 +337,9 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
> >    asm volatile( "tlbie %0"  :: "r"(ea) );
> > }
> >
> > +#endif /* ppc */
> > +#ifdef CONFIG_PPC_64BITSUPPORT
> >
> > static int is_ppc64(void)
> > {
> >    unsigned int pvr;
> > @@ -342,12 +348,21 @@ static int is_ppc64(void)
> >    return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > }
> >
> > +#endif /* CONFIG_PPC_64BITSUPPORT */
>
> static int is_ppc64(void)
> {
> #if defined(__powerpc64__)
>    return 1;
> #elif defined(CONFIG_PPC_64BITSUPPORT)
>    unsigned int pvr = mfpvr();
>    return ((pvr >= 0x330000) && (pvr < 0x70330000));
> #else
>    return 0;
> #endif
> }
> > +/* XXX Drop this function when legacy 64-bit support is dropped. */
> > static void hash_page( unsigned long ea, unsigned long phys, ucell  
> > mode )
> > {
> > +#ifdef __powerpc64__
> > hash_page_64(ea, phys, mode);
> >
> > +#else
> > +#ifdef CONFIG_PPC_64BITSUPPORT
> >    if ( is_ppc64() )
> >    	hash_page_64(ea, phys, mode);
> >    else
> > +#endif
> >    	hash_page_32(ea, phys, mode);
> > +#endif
>
> if (is_ppc64)
>        hash_page_64(ea, phys, mode);
>    else
>        hash_page_32(ea, phys, mode);
> > }
> >
> > void
> > @@ -387,17 +402,27 @@ void
> > setup_mmu( unsigned long ramsize )
> > {
> >    ofmem_t *ofmem;
> > unsigned long sdr1, sr_base;
> > unsigned long sr_base;
> > unsigned long hash_base;
> > unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
> >
> > unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
> > int i;
> >
> > /* SDR1: Storage Description Register 1 */
> >
> > hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> > memset((void *)hash_base, 0, HASH_SIZE);
> > sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> > asm volatile("mtsdr1 %0" :: "r" (sdr1) );
> >
> > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > +#ifdef CONFIG_PPC_64BITSUPPORT
> > if (is_ppc64())
> >
> > +#endif
> > mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> >
> > +#ifdef CONFIG_PPC_64BITSUPPORT
> > else
> >
> > +#endif
> > +#endif
> > +#ifndef __powerpc64__
> > mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
> >
> > +#endif
>
> if (is_ppc64())
>        mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
>    else
>        mtsdr1(hash_base | (HASH_SIZE - 1) >> 16, 0));
>
> That's a lot easier to read, no?

So is

#ifdef __powerpc64__
     mtsdr1(...);
#else
     mtsdr1(...);
#endif

which I'm trying to reach.

In the SLB patch for comparison the final

#ifdef __powerpc64__
     ...
#else
     ...
#endif

code is so much nicer than
     if (is_ppc64())
         ...
     else
         ...

which requires a lot of reindentation to remain readable -  
reindentation that would later be reverted again when legacy support  
is dropped. Trying to avoid that.

Andreas

[PATCH v4] ppc: SDR1 fixes

[Alexander Graf]

On 21.11.2010, at 22:40, Andreas Färber  wrote:
> Am 21.11.2010 um 22:22 schrieb Alexander Graf:
> > On 21.11.2010, at 21:08, Andreas Färber wrote:
> > > Clean up use of SDR1 fields.
> > >
> > > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > > The HTABMASK field in SDR1 contains a mask value that determines
> > > how many bits from the hash are used in the page table index. This mask
> > > must be of the form 0b00...011...1; that is, a string of 0 bits followed by
> > > a string of 1bits. The 1 bits determine how many additional bits (at least 10)
> > > from the hash are used in the index; HTABORG must have this same number of
> > > low-order bits equal to 0.
> > >
> > > IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
> > > The HTABSIZE field in SDR1 contains an integer value that determines
> > > how many bits from the hash are used in the page table index. This number
> > > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > > 0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
> > > The 1-bits determine how many additional bits (beyond the minimum of 11)
> > > from the hash are used in the index. The HTABORG must have this same number
> > > of low-order bits equal to 0.
> > >
> > > Adjust alignment mask and hash size for ppc64.
> > >
> > > Note that the page table does not yet scale with the memory size, and
> > > the secondary hash is not yet used.
> > >
> > > v4:
> > > Don't redefine hash_page_{32,64} but add TODO comments for renaming.
> > > Keep and simplify hash_page().
> > >
> > > v3:
> > > Fix shifts. Thanks to Segher and Alex.
> > > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
> > >
> > > so that it worked as expected. Avoid underflow, spotted by Alex.
> > > Make this fix available for legacy 64-bit support as well.
> > > Drop get_hash_size(). It was only used in hash_page_32() and caused
> > >
> > > unnecessary conversions between HTABMASK and size.
> > > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> > >
> > > Note that this changes the volatility for hash_page_64().
> > > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > > Adjust hash size for ppc64.
> > >
> > > v2:
> > > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> > >
> > > Error spotted by Segher.
> > >
> > > Cc: Alexander Graf 
> > > Cc: Segher Boessenkool 
> > > Signed-off-by: Andreas Färber
> > >
> > > arch/ppc/qemu/ofmem.c        |   69 ++++++++++++++++++++++++++++-------------
> > > include/arch/ppc/processor.h |   12 +++++++
> > > 2 files changed, 59 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > > index 72694b3..6f3079a 100644
> > > --- a/arch/ppc/qemu/ofmem.c
> > > +++ b/arch/ppc/qemu/ofmem.c
> > > @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
> > > #define OF_CODE_START    0xfff00000UL
> > > #define IO_BASE            0x80000000
> > >
> > > -#define HASH_SIZE        (2 << 15)
> > > +#ifdef __powerpc64__
> > > +#define HASH_BITS        18
> > > +#else
> > > +#define HASH_BITS        15
> > > +#endif
> > > +#define HASH_SIZE        (2 << HASH_BITS)
> > > #define OFMEM_SIZE        (2 * 1024 * 1024)
> > >
> > > #define    SEGR_USER        BIT(2)
> > > @@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long code_base );
> > > static inline unsigned long
> > > get_hash_base( void )
> > > {
> > > unsigned long sdr1;
> > > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > > return (sdr1 & 0xffff0000);
> > >
> > > -}
> > >
> > > -static inline unsigned long
> > > -get_hash_size( void )
> > > -{
> > > unsigned long sdr1;
> > > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > > return ((sdr1 << 16) | 0x0000ffff) + 1;
> > >
> > > +#ifdef __powerpc64__
> > > return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
> >
> > Just define these as costants and keep the actual code clean :).
>
> OK.
> > > +#else
> > > return (mfsdr1() & 0xffff0000UL);
> > >
> > > +#endif
> > > }
> > >
> > > static inline unsigned long
> > > @@ -228,6 +223,8 @@ ea_to_phys( ucell ea, ucell *mode )
> > >    return phys;
> > > }
> > >
> > > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > >
> > > static void
> > > hash_page_64( ucell ea, ucell phys, ucell mode )
> > > {
> > > @@ -242,7 +239,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
> > >    vsid = (ea >> 28) + SEGR_BASE;
> > >    vsid_sh = 7;
> > >    vsid_mask = 0x00003FFFFFFFFF80ULL;
> > > asm ( "mfsdr1 %0" : "=r" (sdr) );
> > > sdr = mfsdr1();
> > > sdr_sh = 18;
> > > sdr_mask = 0x3FF80;
> > > page_mask = 0x0FFFFFFF; // XXX correct?
> > >
> > > @@ -294,6 +291,12 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
> > >    asm volatile( "tlbie %0"  :: "r"(ea) );
> > > }
> > >
> > > +#endif /* ppc64 */
> > > +#ifndef __powerpc64__
> > > +/* XXX Turn this into an #else when legacy 64-bit support is gone
> > > and rename hash_page{32,64} to hash_page.
> >
> > Just keep them around. They shouldn't hurt. The code will be optimized away by the compiler anyways if CONFIG_PPC_64BITSUPPORT is not set, as static functions that never get caled (if on a constant 0 is never get called) just don't get compiled in.
>
> They do hurt in that not calling them leads to an error.

Nope.

static void x(void)
{
}

This breaks:

#if 0
x();
#endif

This works:

if (0)
    x();

So if you keep the call to the 32 bit stuff in compiled code that gets optimized out because it can never be called, everybody is happy :).
> > > */
> > >
> > > static void
> > > hash_page_32( ucell ea, ucell phys, ucell mode )
> > > {
> > > @@ -307,7 +310,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
> > >
> > > hash1 = vsid;
> > >    hash1 ^= (ea >> 12) & 0xffff;
> > > hash1 &= (get_hash_size() - 1) >> 6;
> > >
> > > hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
> > >
> > > pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> > > upte = (unsigned long*)pp;
> > >
> > > @@ -334,6 +337,9 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
> > >    asm volatile( "tlbie %0"  :: "r"(ea) );
> > > }
> > >
> > > +#endif /* ppc */
> > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > >
> > > static int is_ppc64(void)
> > > {
> > >    unsigned int pvr;
> > > @@ -342,12 +348,21 @@ static int is_ppc64(void)
> > >    return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > > }
> > >
> > > +#endif /* CONFIG_PPC_64BITSUPPORT */
> >
> > static int is_ppc64(void)
> > {
> > #if defined(__powerpc64__)
> >   return 1;
> > #elif defined(CONFIG_PPC_64BITSUPPORT)
> >   unsigned int pvr = mfpvr();
> >   return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > #else
> >   return 0;
> > #endif
> > }
> > > +/* XXX Drop this function when legacy 64-bit support is dropped. */
> > > static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
> > > {
> > > +#ifdef __powerpc64__
> > > hash_page_64(ea, phys, mode);
> > >
> > > +#else
> > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > >    if ( is_ppc64() )
> > >        hash_page_64(ea, phys, mode);
> > >    else
> > > +#endif
> > >        hash_page_32(ea, phys, mode);
> > > +#endif
> >
> > if (is_ppc64)
> >       hash_page_64(ea, phys, mode);
> >   else
> >       hash_page_32(ea, phys, mode);
> > > }
> > >
> > > void
> > > @@ -387,17 +402,27 @@ void
> > > setup_mmu( unsigned long ramsize )
> > > {
> > >    ofmem_t *ofmem;
> > > unsigned long sdr1, sr_base;
> > > unsigned long sr_base;
> > > unsigned long hash_base;
> > > unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
> > >
> > > unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
> > > int i;
> > >
> > > /* SDR1: Storage Description Register 1 */
> > >
> > > hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> > > memset((void *)hash_base, 0, HASH_SIZE);
> > > sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> > > asm volatile("mtsdr1 %0" :: "r" (sdr1) );
> > >
> > > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > if (is_ppc64())
> > >
> > > +#endif
> > >    mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> > >
> > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > else
> > >
> > > +#endif
> > > +#endif
> > > +#ifndef __powerpc64__
> > >    mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
> > >
> > > +#endif
> >
> > if (is_ppc64())
> >       mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> >   else
> >       mtsdr1(hash_base | (HASH_SIZE - 1) >> 16, 0));
> >
> > That's a lot easier to read, no?
>
> So is
>
> #ifdef __powerpc64__
>    mtsdr1(...);
> #else
>    mtsdr1(...);
> #endif
>
> which I'm trying to reach.
>
> In the SLB patch for comparison the final
>
> #ifdef __powerpc64__
>    ...
> #else
>    ...
> #endif
>
> code is so much nicer than
>    if (is_ppc64())
>        ...
>    else
>        ...
>
> which requires a lot of reindentation to remain readable - reindentation that would later be reverted again when legacy support is dropped. Trying to avoid that.

For in total 10 lines of code?

Why are you trying so hard to get rid of legacy ppc64 compat? It doesn't hurt to have it around as long as you keep the #ifdefs to a reasonable minimum :). By then it's actually nice to have for code check reasons: compile ppc64 and be reasonably sure you don't break ppc32.

Alex
>

[PATCH v4] ppc: SDR1 fixes

[Andreas Färber]

Am 21.11.2010 um 22:53 schrieb Alexander Graf:
> On 21.11.2010, at 22:40, Andreas Färber   
> wrote:
> > Am 21.11.2010 um 22:22 schrieb Alexander Graf:
> > > On 21.11.2010, at 21:08, Andreas Färber wrote:
> > > > Clean up use of SDR1 fields.
> > > >
> > > > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > > > The HTABMASK field in SDR1 contains a mask value that determines
> > > > how many bits from the hash are used in the page table index.  
> > > > This mask
> > > > must be of the form 0b00...011...1; that is, a string of 0 bits  
> > > > followed by
> > > > a string of 1bits. The 1 bits determine how many additional bits  
> > > > (at least 10)
> > > > from the hash are used in the index; HTABORG must have this same  
> > > > number of
> > > > low-order bits equal to 0.
> > > >
> > > > IBM's "The Programming Environments Manual for 64-bit  
> > > > Microprocessors" 3.0:
> > > > The HTABSIZE field in SDR1 contains an integer value that  
> > > > determines
> > > > how many bits from the hash are used in the page table index.  
> > > > This number
> > > > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > > > 0b00...011...1; that is, a string of 0 bits followed by a string  
> > > > of 1-bits.
> > > > The 1-bits determine how many additional bits (beyond the minimum  
> > > > of 11)
> > > > from the hash are used in the index. The HTABORG must have this  
> > > > same number
> > > > of low-order bits equal to 0.
> > > >
> > > > Adjust alignment mask and hash size for ppc64.
> > > >
> > > > Note that the page table does not yet scale with the memory size,  
> > > > and
> > > > the secondary hash is not yet used.
> > > >
> > > > v4:
> > > > Don't redefine hash_page_{32,64} but add TODO comments for
> > > >
> > > > renaming.
> > > > Keep and simplify hash_page().
> > > >
> > > > v3:
> > > > Fix shifts. Thanks to Segher and Alex.
> > > > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened
> > > >
> > > > to be 0,
> > > > so that it worked as expected. Avoid underflow, spotted by Alex.
> > > > Make this fix available for legacy 64-bit support as well.
> > > > Drop get_hash_size(). It was only used in hash_page_32() and
> > > >
> > > > caused
> > > > unnecessary conversions between HTABMASK and size.
> > > > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> > > >
> > > > Note that this changes the volatility for hash_page_64().
> > > > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > > > Adjust hash size for ppc64.
> > > >
> > > > v2:
> > > > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> > > >
> > > > Error spotted by Segher.
> > > >
> > > > Cc: Alexander Graf 
> > > > Cc: Segher Boessenkool 
> > > > Signed-off-by: Andreas Färber
> > > >
> > > > arch/ppc/qemu/ofmem.c        |   69 +++++++++++++++++++++++++++ 
> > > > +-------------
> > > > include/arch/ppc/processor.h |   12 +++++++
> > > > 2 files changed, 59 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > > > index 72694b3..6f3079a 100644
> > > > --- a/arch/ppc/qemu/ofmem.c
> > > > +++ b/arch/ppc/qemu/ofmem.c
> > > > @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long  
> > > > code_base );
> > > > #define OF_CODE_START    0xfff00000UL
> > > > #define IO_BASE            0x80000000
> > > >
> > > > -#define HASH_SIZE        (2 << 15)
> > > > +#ifdef __powerpc64__
> > > > +#define HASH_BITS        18
> > > > +#else
> > > > +#define HASH_BITS        15
> > > > +#endif
> > > > +#define HASH_SIZE        (2 << HASH_BITS)
> > > > #define OFMEM_SIZE        (2 * 1024 * 1024)
> > > >
> > > > #define    SEGR_USER        BIT(2)
> > > > @@ -57,21 +62,11 @@ extern void setup_mmu( unsigned long  
> > > > code_base );
> > > > static inline unsigned long
> > > > get_hash_base( void )
> > > > {
> > > > unsigned long sdr1;
> > > > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > > > return (sdr1 & 0xffff0000);
> > > >
> > > > -}
> > > >
> > > > -static inline unsigned long
> > > > -get_hash_size( void )
> > > > -{
> > > > unsigned long sdr1;
> > > > asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> > > > return ((sdr1 << 16) | 0x0000ffff) + 1;
> > > >
> > > > +#ifdef __powerpc64__
> > > > return (mfsdr1() & 0x3FFFFFFFFFFC0000UL);
> > >
> > > Just define these as costants and keep the actual code clean :).
> >
> > OK.
> > > > +#else
> > > > return (mfsdr1() & 0xffff0000UL);
> > > >
> > > > +#endif
> > > > }
> > > >
> > > > static inline unsigned long
> > > > @@ -228,6 +223,8 @@ ea_to_phys( ucell ea, ucell *mode )
> > > >   return phys;
> > > > }
> > > >
> > > > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > > >
> > > > static void
> > > > hash_page_64( ucell ea, ucell phys, ucell mode )
> > > > {
> > > > @@ -242,7 +239,7 @@ hash_page_64( ucell ea, ucell phys, ucell  
> > > > mode )
> > > >   vsid = (ea >> 28) + SEGR_BASE;
> > > >   vsid_sh = 7;
> > > >   vsid_mask = 0x00003FFFFFFFFF80ULL;
> > > > asm ( "mfsdr1 %0" : "=r" (sdr) );
> > > > sdr = mfsdr1();
> > > > sdr_sh = 18;
> > > > sdr_mask = 0x3FF80;
> > > > page_mask = 0x0FFFFFFF; // XXX correct?
> > > >
> > > > @@ -294,6 +291,12 @@ hash_page_64( ucell ea, ucell phys, ucell  
> > > > mode )
> > > >   asm volatile( "tlbie %0"  :: "r"(ea) );
> > > > }
> > > >
> > > > +#endif /* ppc64 */
> > > > +#ifndef __powerpc64__
> > > > +/* XXX Turn this into an #else when legacy 64-bit support is gone
> > > > and rename hash_page{32,64} to hash_page.
> > >
> > > Just keep them around. They shouldn't hurt. The code will be  
> > > optimized away by the compiler anyways if CONFIG_PPC_64BITSUPPORT  
> > > is not set, as static functions that never get caled (if on a  
> > > constant 0 is never get called) just don't get compiled in.
> >
> > They do hurt in that not calling them leads to an error.
>
> Nope.
>
> static void x(void)
> {
> }
>
> This breaks:
>
> #if 0
> x();
> #endif
>
> This works:
>
> if (0)
>    x();
>
> So if you keep the call to the 32 bit stuff in compiled code that  
> gets optimized out because it can never be called, everybody is  
> happy :).

I do and did understand that. Make that everybody excluding me.
> > > > */
> > > >
> > > > static void
> > > > hash_page_32( ucell ea, ucell phys, ucell mode )
> > > > {
> > > > @@ -307,7 +310,7 @@ hash_page_32( ucell ea, ucell phys, ucell  
> > > > mode )
> > > >
> > > > hash1 = vsid;
> > > >   hash1 ^= (ea >> 12) & 0xffff;
> > > > hash1 &= (get_hash_size() - 1) >> 6;
> > > >
> > > > hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
> > > >
> > > > pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> > > > upte = (unsigned long*)pp;
> > > >
> > > > @@ -334,6 +337,9 @@ hash_page_32( ucell ea, ucell phys, ucell  
> > > > mode )
> > > >   asm volatile( "tlbie %0"  :: "r"(ea) );
> > > > }
> > > >
> > > > +#endif /* ppc */
> > > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > >
> > > > static int is_ppc64(void)
> > > > {
> > > >   unsigned int pvr;
> > > > @@ -342,12 +348,21 @@ static int is_ppc64(void)
> > > >   return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > > > }
> > > >
> > > > +#endif /* CONFIG_PPC_64BITSUPPORT */
> > >
> > > static int is_ppc64(void)
> > > {
> > > #if defined(__powerpc64__)
> > >  return 1;
> > > #elif defined(CONFIG_PPC_64BITSUPPORT)
> > >  unsigned int pvr = mfpvr();
> > >  return ((pvr >= 0x330000) && (pvr < 0x70330000));
> > > #else
> > >  return 0;
> > > #endif
> > > }
> > > > +/* XXX Drop this function when legacy 64-bit support is dropped.  
> > > > */
> > > > static void hash_page( unsigned long ea, unsigned long phys,  
> > > > ucell mode )
> > > > {
> > > > +#ifdef __powerpc64__
> > > > hash_page_64(ea, phys, mode);
> > > >
> > > > +#else
> > > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > >   if ( is_ppc64() )
> > > >       hash_page_64(ea, phys, mode);
> > > >   else
> > > > +#endif
> > > >       hash_page_32(ea, phys, mode);
> > > > +#endif
> > >
> > > if (is_ppc64)
> > >      hash_page_64(ea, phys, mode);
> > >  else
> > >      hash_page_32(ea, phys, mode);
> > > > }
> > > >
> > > > void
> > > > @@ -387,17 +402,27 @@ void
> > > > setup_mmu( unsigned long ramsize )
> > > > {
> > > >   ofmem_t *ofmem;
> > > > unsigned long sdr1, sr_base;
> > > > unsigned long sr_base;
> > > > unsigned long hash_base;
> > > > unsigned long hash_mask = 0xfff00000; /* alignment for ppc64
> > > >
> > > > */
> > > > unsigned long hash_mask = ~0x000fffffUL; /* alignment for
> > > >
> > > > ppc64 */
> > > >   int i;
> > > >
> > > > /* SDR1: Storage Description Register 1 */
> > > >
> > > > hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> > > >   memset((void *)hash_base, 0, HASH_SIZE);
> > > > sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> > > > asm volatile("mtsdr1 %0" :: "r" (sdr1) );
> > > >
> > > > +#if defined(__powerpc64__) || defined(CONFIG_PPC_64BITSUPPORT)
> > > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > > if (is_ppc64())
> > > >
> > > > +#endif
> > > >    mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> > > >
> > > > +#ifdef CONFIG_PPC_64BITSUPPORT
> > > > else
> > > >
> > > > +#endif
> > > > +#endif
> > > > +#ifndef __powerpc64__
> > > >    mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
> > > >
> > > > +#endif
> > >
> > > if (is_ppc64())
> > >      mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> > >  else
> > >      mtsdr1(hash_base | (HASH_SIZE - 1) >> 16, 0));
> > >
> > > That's a lot easier to read, no?
> >
> > So is
> >
> > #ifdef __powerpc64__
> >   mtsdr1(...);
> > #else
> >   mtsdr1(...);
> > #endif
> >
> > which I'm trying to reach.
> >
> > In the SLB patch for comparison the final
> >
> > #ifdef __powerpc64__
> >   ...
> > #else
> >   ...
> > #endif
> >
> > code is so much nicer than
> >   if (is_ppc64())
> >       ...
> >   else
> >       ...
> >
> > which requires a lot of reindentation to remain readable -  
> > reindentation that would later be reverted again when legacy  
> > support is dropped. Trying to avoid that.
>
> For in total 10 lines of code?
>
> Why are you trying so hard to get rid of legacy ppc64 compat? It  
> doesn't hurt to have it around as long as you keep the #ifdefs to a  
> reasonable minimum :). By then it's actually nice to have for code  
> check reasons: compile ppc64 and be reasonably sure you don't break  
> ppc32.

Nope. ppc code uses unsigned long and expects it to be 32 bits. All  
instances of such code will get compiled with sizes it was not  
intended for and it causes to loose time fiddling with code that I  
don't need to touch. This hit us before (e.g., get_hash_size) and will  
do so in the future. If code is designed for one bitness, don't force  
unnecessarily compiling it for another, even if the linker might  
optimize it out *after* successful compilation.

Compiling ppc64 code allows no definite conclusions about ppc.

Rebasing it now and will look for a solution.

Andreas

[PATCH v5] ppc: SDR1 fixes

[Andreas Färber]

Clean up use of SDR1 fields.

IBM's "The Programming Environments for 32-Bit Microprocessors":
  The HTABMASK field in SDR1 contains a mask value that determines
  how many bits from the hash are used in the page table index. This mask
  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
  from the hash are used in the index; HTABORG must have this same number of
  low-order bits equal to 0.

IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
  The HTABSIZE field in SDR1 contains an integer value that determines
  how many bits from the hash are used in the page table index. This number
  must not exceed 28. HTABSIZE is used to generate a mask of the form
  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
  The 1-bits determine how many additional bits (beyond the minimum of 11)
  from the hash are used in the index. The HTABORG must have this same number
  of low-order bits equal to 0.

Adjust alignment mask and hash size for ppc64.

Note that the page table does not yet scale with the memory size, and
the secondary hash is not yet used.

v5:
* Move HTABORG mask to header.
* Keep is_ppc64() for now, suggested by Alex.
  Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as intended,
  not recognizing a ppc64 CPU in pure 32-bit mode.
* Stub out hash_page_32() for ppc64 to avoid having to care about it for ppc64.
  hash_page_64() is less critical for pure ppc build since the code needs to be
  32-bit compatible anyway.

v4:
* Don't redefine hash_page_{32,64} but add TODO comments for renaming.
* Keep and simplify hash_page().

v3:
* Fix shifts. Thanks to Segher and Alex.
* Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
  so that it worked as expected. Avoid underflow, spotted by Alex.
  Make this fix available for legacy 64-bit support as well.
* Drop get_hash_size(). It was only used in hash_page_32() and caused
  unnecessary conversions between HTABMASK and size.
* Introduce {mfsdr1,mtsdr1} as suggested by Alex.
  Note that this changes the volatility for hash_page_64().
* Conditionalize 64-bit support, drop 32-bit support for ppc64.
* Adjust hash size for ppc64.

v2:
* Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
  Error spotted by Segher.

Cc: Alexander Graf 
Cc: Segher Boessenkool 
Signed-off-by: Andreas Färber 
---
 arch/ppc/qemu/ofmem.c        |   45 +++++++++++++++++++++--------------------
 include/arch/ppc/processor.h |   18 ++++++++++++++++
 2 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index e871623..a844ea7 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
 #define OF_CODE_START	0xfff00000UL
 #define IO_BASE			0x80000000

-#define HASH_SIZE		(2 << 15)
+#ifdef __powerpc64__
+#define HASH_BITS		18
+#else
+#define HASH_BITS		15
+#endif
+#define HASH_SIZE		(2 << HASH_BITS)
 #define OFMEM_SIZE		(2 * 1024 * 1024)

#define	SEGR_USER		BIT(2)
@@ -57,21 +62,7 @@ extern void setup_mmu( unsigned long code_base );
 static inline unsigned long
 get_hash_base( void )
 {
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return (sdr1 & 0xffff0000);
-}
-
-static inline unsigned long
-get_hash_size( void )
-{
-	unsigned long sdr1;
-
-	asm volatile("mfsdr1 %0" : "=r" (sdr1) );
-
-	return ((sdr1 << 16) | 0x0000ffff) + 1;
+    return (mfsdr1() & SDR1_HTABORG_MASK);
 }

static inline unsigned long
@@ -242,7 +233,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
    vsid = (ea >> 28) + SEGR_BASE;
    vsid_sh = 7;
    vsid_mask = 0x00003FFFFFFFFF80ULL;
-	asm ( "mfsdr1 %0" : "=r" (sdr) );
+	sdr = mfsdr1();
    sdr_sh = 18;
    sdr_mask = 0x3FF80;
    page_mask = 0x0FFFFFFF; // XXX correct?
@@ -297,6 +288,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
 static void
 hash_page_32( ucell ea, ucell phys, ucell mode )
 {
+#ifndef __powerpc64__
    static int next_grab_slot=0;
    unsigned long *upte, cmp, hash1;
    int i, vsid, found;
@@ -307,7 +299,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )

hash1 = vsid;
    hash1 ^= (ea >> 12) & 0xffff;
-	hash1 &= (get_hash_size() - 1) >> 6;
+	hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;

pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
    upte = (unsigned long*)pp;
@@ -332,14 +324,22 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
    upte[i*2+1] = (phys & ~0xfff) | mode;

asm volatile( "tlbie %0"  :: "r"(ea) );
+#endif
 }

static int is_ppc64(void)
 {
+#ifdef __powerpc64__
+	return 1;
+#elif defined(CONFIG_PPC_64BITSUPPORT)
    unsigned int pvr = mfpvr();
    return ((pvr >= 0x330000) && (pvr < 0x70330000));
+#else
+	return 0;
+#endif
 }

+/* XXX Remove these ugly constructs when legacy 64-bit support is dropped. */
 static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
 {
    if ( is_ppc64() )
@@ -385,20 +385,21 @@ void
 setup_mmu( unsigned long ramsize )
 {
    ofmem_t *ofmem;
-	unsigned long sdr1;
 #ifndef __powerpc64__
    unsigned long sr_base;
 #endif
    unsigned long hash_base;
-	unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
+	unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
    int i;

/* SDR1: Storage Description Register 1 */

hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
    memset((void *)hash_base, 0, HASH_SIZE);
-	sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
-	asm volatile("mtsdr1 %0" :: "r" (sdr1) );
+	if (is_ppc64())
+		mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
+	else
+		mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));

#ifdef __powerpc64__

diff --git a/include/arch/ppc/processor.h b/include/arch/ppc/processor.h
index c2f1284..21e4fab 100644
--- a/include/arch/ppc/processor.h
+++ b/include/arch/ppc/processor.h
@@ -425,6 +425,24 @@ static inline void mtmsr(unsigned long msr)
 #endif
 }

+#ifdef __powerpc64__
+#define SDR1_HTABORG_MASK 0x3FFFFFFFFFFC0000UL
+#else
+#define SDR1_HTABORG_MASK 0xffff0000
+#endif
+
+static inline unsigned long mfsdr1(void)
+{
+    unsigned long sdr1;
+    asm volatile("mfsdr1 %0" : "=r" (sdr1));
+    return sdr1;
+}
+
+static inline void mtsdr1(unsigned long sdr1)
+{
+    asm volatile("mtsdr1 %0" :: "r" (sdr1));
+}
+
 static inline unsigned int mfpvr(void)
 {
     unsigned int pvr;

[PATCH v5] ppc: SDR1 fixes

[Alexander Graf]

On 22.11.2010, at 00:14, Andreas Färber  wrote:
> Clean up use of SDR1 fields.
>
> IBM's "The Programming Environments for 32-Bit Microprocessors":
>  The HTABMASK field in SDR1 contains a mask value that determines
>  how many bits from the hash are used in the page table index. This mask
>  must be of the form 0b00...011...1; that is, a string of 0 bits followed by
>  a string of 1bits. The 1 bits determine how many additional bits (at least 10)
>  from the hash are used in the index; HTABORG must have this same number of
>  low-order bits equal to 0.
>
> IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
>  The HTABSIZE field in SDR1 contains an integer value that determines
>  how many bits from the hash are used in the page table index. This number
>  must not exceed 28. HTABSIZE is used to generate a mask of the form
>  0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
>  The 1-bits determine how many additional bits (beyond the minimum of 11)
>  from the hash are used in the index. The HTABORG must have this same number
>  of low-order bits equal to 0.
>
> Adjust alignment mask and hash size for ppc64.
>
> Note that the page table does not yet scale with the memory size, and
> the secondary hash is not yet used.
>
> v5:
> Move HTABORG mask to header.
> Keep is_ppc64() for now, suggested by Alex.
>
> Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as intended,
>  not recognizing a ppc64 CPU in pure 32-bit mode.
> Stub out hash_page_32() for ppc64 to avoid having to care about it for ppc64.
>
> hash_page_64() is less critical for pure ppc build since the code needs to be
>  32-bit compatible anyway.
>
> v4:
> Don't redefine hash_page_{32,64} but add TODO comments for renaming.
> Keep and simplify hash_page().
>
> v3:
> Fix shifts. Thanks to Segher and Alex.
> Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
>
> so that it worked as expected. Avoid underflow, spotted by Alex.
>  Make this fix available for legacy 64-bit support as well.
> Drop get_hash_size(). It was only used in hash_page_32() and caused
>
> unnecessary conversions between HTABMASK and size.
> Introduce {mfsdr1,mtsdr1} as suggested by Alex.
>
> Note that this changes the volatility for hash_page_64().
> Conditionalize 64-bit support, drop 32-bit support for ppc64.
> Adjust hash size for ppc64.
>
> v2:
> Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
>
> Error spotted by Segher.
>
> Cc: Alexander Graf 
> Cc: Segher Boessenkool 
> Signed-off-by: Andreas Färber

This one looks a lot cleaner. I don't see any obvious glitches either, so:

Signed-off-by: Alexander Graf
> arch/ppc/qemu/ofmem.c        |   45 +++++++++++++++++++++--------------------
> include/arch/ppc/processor.h |   18 ++++++++++++++++
> 2 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> index e871623..a844ea7 100644
> --- a/arch/ppc/qemu/ofmem.c
> +++ b/arch/ppc/qemu/ofmem.c
> @@ -48,7 +48,12 @@ extern void setup_mmu( unsigned long code_base );
> #define OF_CODE_START    0xfff00000UL
> #define IO_BASE            0x80000000
>
> -#define HASH_SIZE        (2 << 15)
> +#ifdef __powerpc64__
> +#define HASH_BITS        18
> +#else
> +#define HASH_BITS        15
> +#endif
> +#define HASH_SIZE        (2 << HASH_BITS)
> #define OFMEM_SIZE        (2 * 1024 * 1024)
>
> #define    SEGR_USER        BIT(2)
> @@ -57,21 +62,7 @@ extern void setup_mmu( unsigned long code_base );
> static inline unsigned long
> get_hash_base( void )
> {
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return (sdr1 & 0xffff0000);
>
> -}
>
> -static inline unsigned long
> -get_hash_size( void )
> -{
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1) );
> return ((sdr1 << 16) | 0x0000ffff) + 1;
> return (mfsdr1() & SDR1_HTABORG_MASK);
>
> }
>
> static inline unsigned long
> @@ -242,7 +233,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
>    vsid = (ea >> 28) + SEGR_BASE;
>    vsid_sh = 7;
>    vsid_mask = 0x00003FFFFFFFFF80ULL;
> asm ( "mfsdr1 %0" : "=r" (sdr) );
> sdr = mfsdr1();
> sdr_sh = 18;
> sdr_mask = 0x3FF80;
> page_mask = 0x0FFFFFFF; // XXX correct?
>
> @@ -297,6 +288,7 @@ hash_page_64( ucell ea, ucell phys, ucell mode )
> static void
> hash_page_32( ucell ea, ucell phys, ucell mode )
> {
> +#ifndef __powerpc64__
>    static int next_grab_slot=0;
>    unsigned long *upte, cmp, hash1;
>    int i, vsid, found;
> @@ -307,7 +299,7 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>
> hash1 = vsid;
>    hash1 ^= (ea >> 12) & 0xffff;
> hash1 &= (get_hash_size() - 1) >> 6;
>
> hash1 &= (((mfsdr1() & 0x1ff) << 16) | 0xffff) >> 6;
>
> pp = (mPTE_t*)(get_hash_base() + (hash1 << 6));
> upte = (unsigned long*)pp;
>
> @@ -332,14 +324,22 @@ hash_page_32( ucell ea, ucell phys, ucell mode )
>    upte[i*2+1] = (phys & ~0xfff) | mode;
>
> asm volatile( "tlbie %0"  :: "r"(ea) );
> +#endif
> }
>
> static int is_ppc64(void)
> {
> +#ifdef __powerpc64__
> return 1;
>
> +#elif defined(CONFIG_PPC_64BITSUPPORT)
>    unsigned int pvr = mfpvr();
>    return ((pvr >= 0x330000) && (pvr < 0x70330000));
> +#else
> return 0;
>
> +#endif
> }
>
> +/* XXX Remove these ugly constructs when legacy 64-bit support is dropped. */
> static void hash_page( unsigned long ea, unsigned long phys, ucell mode )
> {
>    if ( is_ppc64() )
> @@ -385,20 +385,21 @@ void
> setup_mmu( unsigned long ramsize )
> {
>    ofmem_t *ofmem;
> unsigned long sdr1;
>
> #ifndef __powerpc64__
>    unsigned long sr_base;
> #endif
>    unsigned long hash_base;
> unsigned long hash_mask = 0xfff00000; /* alignment for ppc64 */
>
> unsigned long hash_mask = ~0x000fffffUL; /* alignment for ppc64 */
> int i;
>
> /* SDR1: Storage Description Register 1 */
>
> hash_base = (ramsize - 0x00100000 - HASH_SIZE) & hash_mask;
> memset((void *)hash_base, 0, HASH_SIZE);
> sdr1 = hash_base | ((HASH_SIZE-1) >> 16);
> asm volatile("mtsdr1 %0" :: "r" (sdr1) );
> if (is_ppc64())
>    mtsdr1(hash_base | MAX(HASH_BITS - 18, 0));
> else
>    mtsdr1(hash_base | ((HASH_SIZE - 1) >> 16));
>
> #ifdef __powerpc64__
>
> diff --git a/include/arch/ppc/processor.h b/include/arch/ppc/processor.h
> index c2f1284..21e4fab 100644
> --- a/include/arch/ppc/processor.h
> +++ b/include/arch/ppc/processor.h
> @@ -425,6 +425,24 @@ static inline void mtmsr(unsigned long msr)
> #endif
> }
>
> +#ifdef __powerpc64__
> +#define SDR1_HTABORG_MASK 0x3FFFFFFFFFFC0000UL
> +#else
> +#define SDR1_HTABORG_MASK 0xffff0000
> +#endif
>
> +static inline unsigned long mfsdr1(void)
> +{
> unsigned long sdr1;
> asm volatile("mfsdr1 %0" : "=r" (sdr1));
> return sdr1;
>
> +}
>
> +static inline void mtsdr1(unsigned long sdr1)
> +{
> asm volatile("mtsdr1 %0" :: "r" (sdr1));
>
> +}
>
> static inline unsigned int mfpvr(void)
> {
>     unsigned int pvr;
> -- 
> 1.7.3

[PATCH v5] ppc: SDR1 fixes

[Andreas Färber]

Am 22.11.2010 um 00:46 schrieb Alexander Graf:
> On 22.11.2010, at 00:14, Andreas Färber   
> wrote:
> > Clean up use of SDR1 fields.
> >
> > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > The HTABMASK field in SDR1 contains a mask value that determines
> > how many bits from the hash are used in the page table index. This  
> > mask
> > must be of the form 0b00...011...1; that is, a string of 0 bits  
> > followed by
> > a string of 1bits. The 1 bits determine how many additional bits  
> > (at least 10)
> > from the hash are used in the index; HTABORG must have this same  
> > number of
> > low-order bits equal to 0.
> >
> > IBM's "The Programming Environments Manual for 64-bit  
> > Microprocessors" 3.0:
> > The HTABSIZE field in SDR1 contains an integer value that determines
> > how many bits from the hash are used in the page table index. This  
> > number
> > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > 0b00...011...1; that is, a string of 0 bits followed by a string of  
> > 1-bits.
> > The 1-bits determine how many additional bits (beyond the minimum  
> > of 11)
> > from the hash are used in the index. The HTABORG must have this  
> > same number
> > of low-order bits equal to 0.
> >
> > Adjust alignment mask and hash size for ppc64.
> >
> > Note that the page table does not yet scale with the memory size, and
> > the secondary hash is not yet used.
> >
> > v5:
> > Move HTABORG mask to header.
> > Keep is_ppc64() for now, suggested by Alex.
> >
> > Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as  
> > intended,
> > not recognizing a ppc64 CPU in pure 32-bit mode.
> > Stub out hash_page_32() for ppc64 to avoid having to care about
> >
> > it for ppc64.
> > hash_page_64() is less critical for pure ppc build since the code  
> > needs to be
> > 32-bit compatible anyway.
> >
> > v4:
> > Don't redefine hash_page_{32,64} but add TODO comments for
> >
> > renaming.
> > Keep and simplify hash_page().
> >
> > v3:
> > Fix shifts. Thanks to Segher and Alex.
> > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to
> >
> > be 0,
> > so that it worked as expected. Avoid underflow, spotted by Alex.
> > Make this fix available for legacy 64-bit support as well.
> > Drop get_hash_size(). It was only used in hash_page_32() and caused
> >
> > unnecessary conversions between HTABMASK and size.
> > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> >
> > Note that this changes the volatility for hash_page_64().
> > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > Adjust hash size for ppc64.
> >
> > v2:
> > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> >
> > Error spotted by Segher.
> >
> > Cc: Alexander Graf 
> > Cc: Segher Boessenkool 
> > Signed-off-by: Andreas Färber
>
> This one looks a lot cleaner. I don't see any obvious glitches  
> either, so:
>
> Signed-off-by: Alexander Graf

Phew... thanks! Applied as r967.

A rather unproductive weekend unfortunately, no progress in gdb yet...

Andreas

[PATCH v5] ppc: SDR1 fixes

[Alexander Graf]

On 22.11.2010, at 21:20, Andreas Färber wrote:
> Am 22.11.2010 um 00:46 schrieb Alexander Graf:
> > On 22.11.2010, at 00:14, Andreas Färber  wrote:
> > > Clean up use of SDR1 fields.
> > >
> > > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > > The HTABMASK field in SDR1 contains a mask value that determines
> > > how many bits from the hash are used in the page table index. This mask
> > > must be of the form 0b00...011...1; that is, a string of 0 bits followed by
> > > a string of 1bits. The 1 bits determine how many additional bits (at least 10)
> > > from the hash are used in the index; HTABORG must have this same number of
> > > low-order bits equal to 0.
> > >
> > > IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
> > > The HTABSIZE field in SDR1 contains an integer value that determines
> > > how many bits from the hash are used in the page table index. This number
> > > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > > 0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
> > > The 1-bits determine how many additional bits (beyond the minimum of 11)
> > > from the hash are used in the index. The HTABORG must have this same number
> > > of low-order bits equal to 0.
> > >
> > > Adjust alignment mask and hash size for ppc64.
> > >
> > > Note that the page table does not yet scale with the memory size, and
> > > the secondary hash is not yet used.
> > >
> > > v5:
> > > Move HTABORG mask to header.
> > > Keep is_ppc64() for now, suggested by Alex.
> > >
> > > Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as intended,
> > > not recognizing a ppc64 CPU in pure 32-bit mode.
> > > Stub out hash_page_32() for ppc64 to avoid having to care about it for ppc64.
> > >
> > > hash_page_64() is less critical for pure ppc build since the code needs to be
> > > 32-bit compatible anyway.
> > >
> > > v4:
> > > Don't redefine hash_page_{32,64} but add TODO comments for renaming.
> > > Keep and simplify hash_page().
> > >
> > > v3:
> > > Fix shifts. Thanks to Segher and Alex.
> > > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
> > >
> > > so that it worked as expected. Avoid underflow, spotted by Alex.
> > > Make this fix available for legacy 64-bit support as well.
> > > Drop get_hash_size(). It was only used in hash_page_32() and caused
> > >
> > > unnecessary conversions between HTABMASK and size.
> > > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> > >
> > > Note that this changes the volatility for hash_page_64().
> > > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > > Adjust hash size for ppc64.
> > >
> > > v2:
> > > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> > >
> > > Error spotted by Segher.
> > >
> > > Cc: Alexander Graf 
> > > Cc: Segher Boessenkool 
> > > Signed-off-by: Andreas Färber
> >
> > This one looks a lot cleaner. I don't see any obvious glitches either, so:
> >
> > Signed-off-by: Alexander Graf
>
> Phew... thanks! Applied as r967.
>
> A rather unproductive weekend unfortunately, no progress in gdb yet...

Sorry for making you go through so many iterations :). One thing came to mind still: Please make sure that you take into account that the page table size is different for ppc64-on-ppc32 than the define inclines. If anything needs to know the real htab size, you need to have that one call a function that find it out for real.

Alex

[PATCH v5] ppc: SDR1 fixes

[Andreas Färber]

Am 22.11.2010 um 21:31 schrieb Alexander Graf:
> On 22.11.2010, at 21:20, Andreas Färber wrote:
> > Am 22.11.2010 um 00:46 schrieb Alexander Graf:
> > > On 22.11.2010, at 00:14, Andreas Färber   
> > > wrote:
> > > > Clean up use of SDR1 fields.
> > > >
> > > > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > > > The HTABMASK field in SDR1 contains a mask value that determines
> > > > how many bits from the hash are used in the page table index.  
> > > > This mask
> > > > must be of the form 0b00...011...1; that is, a string of 0 bits  
> > > > followed by
> > > > a string of 1bits. The 1 bits determine how many additional bits  
> > > > (at least 10)
> > > > from the hash are used in the index; HTABORG must have this same  
> > > > number of
> > > > low-order bits equal to 0.
> > > >
> > > > IBM's "The Programming Environments Manual for 64-bit  
> > > > Microprocessors" 3.0:
> > > > The HTABSIZE field in SDR1 contains an integer value that  
> > > > determines
> > > > how many bits from the hash are used in the page table index.  
> > > > This number
> > > > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > > > 0b00...011...1; that is, a string of 0 bits followed by a string  
> > > > of 1-bits.
> > > > The 1-bits determine how many additional bits (beyond the minimum  
> > > > of 11)
> > > > from the hash are used in the index. The HTABORG must have this  
> > > > same number
> > > > of low-order bits equal to 0.
> > > >
> > > > Adjust alignment mask and hash size for ppc64.
> > > >
> > > > Note that the page table does not yet scale with the memory size,  
> > > > and
> > > > the secondary hash is not yet used.
> > > >
> > > > v5:
> > > > Move HTABORG mask to header.
> > > > Keep is_ppc64() for now, suggested by Alex.
> > > >
> > > > Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as  
> > > > intended,
> > > > not recognizing a ppc64 CPU in pure 32-bit mode.
> > > > Stub out hash_page_32() for ppc64 to avoid having to care about
> > > >
> > > > it for ppc64.
> > > > hash_page_64() is less critical for pure ppc build since the code  
> > > > needs to be
> > > > 32-bit compatible anyway.
> > > >
> > > > v4:
> > > > Don't redefine hash_page_{32,64} but add TODO comments for
> > > >
> > > > renaming.
> > > > Keep and simplify hash_page().
> > > >
> > > > v3:
> > > > Fix shifts. Thanks to Segher and Alex.
> > > > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened
> > > >
> > > > to be 0,
> > > > so that it worked as expected. Avoid underflow, spotted by Alex.
> > > > Make this fix available for legacy 64-bit support as well.
> > > > Drop get_hash_size(). It was only used in hash_page_32() and
> > > >
> > > > caused
> > > > unnecessary conversions between HTABMASK and size.
> > > > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> > > >
> > > > Note that this changes the volatility for hash_page_64().
> > > > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > > > Adjust hash size for ppc64.
> > > >
> > > > v2:
> > > > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> > > >
> > > > Error spotted by Segher.
> > > >
> > > > Cc: Alexander Graf 
> > > > Cc: Segher Boessenkool 
> > > > Signed-off-by: Andreas Färber
> > >
> > > This one looks a lot cleaner. I don't see any obvious glitches  
> > > either, so:
> > >
> > > Signed-off-by: Alexander Graf
> >
> > Phew... thanks! Applied as r967.
> >
> > A rather unproductive weekend unfortunately, no progress in gdb  
> > yet...
>
> Sorry for making you go through so many iterations :).

I'll be stricter in the future, you too have commit rights, to create  
the inline functions you like. :)
> One thing came to mind still: Please make sure that you take into  
> account that the page table size is different for ppc64-on-ppc32  
> than the define inclines. If anything needs to know the real htab  
> size, you need to have that one call a function that find it out for  
> real.

You mean the 15 that our MAX() turns into 18? Our code always reads  
HTABORG and HTABMASK/HTABSIZE from SDR1 so we should be just fine once  
it's correctly initialized.
As noted, we should scale the page table with RAM size and use these  
defines as lower limit only.

Andreas

[PATCH v5] ppc: SDR1 fixes

[Alexander Graf]

On 22.11.2010, at 21:42, Andreas Färber wrote:
> Am 22.11.2010 um 21:31 schrieb Alexander Graf:
> > On 22.11.2010, at 21:20, Andreas Färber wrote:
> > > Am 22.11.2010 um 00:46 schrieb Alexander Graf:
> > > > On 22.11.2010, at 00:14, Andreas Färber  wrote:
> > > > > Clean up use of SDR1 fields.
> > > > >
> > > > > IBM's "The Programming Environments for 32-Bit Microprocessors":
> > > > > The HTABMASK field in SDR1 contains a mask value that determines
> > > > > how many bits from the hash are used in the page table index. This mask
> > > > > must be of the form 0b00...011...1; that is, a string of 0 bits followed by
> > > > > a string of 1bits. The 1 bits determine how many additional bits (at least 10)
> > > > > from the hash are used in the index; HTABORG must have this same number of
> > > > > low-order bits equal to 0.
> > > > >
> > > > > IBM's "The Programming Environments Manual for 64-bit Microprocessors" 3.0:
> > > > > The HTABSIZE field in SDR1 contains an integer value that determines
> > > > > how many bits from the hash are used in the page table index. This number
> > > > > must not exceed 28. HTABSIZE is used to generate a mask of the form
> > > > > 0b00...011...1; that is, a string of 0 bits followed by a string of 1-bits.
> > > > > The 1-bits determine how many additional bits (beyond the minimum of 11)
> > > > > from the hash are used in the index. The HTABORG must have this same number
> > > > > of low-order bits equal to 0.
> > > > >
> > > > > Adjust alignment mask and hash size for ppc64.
> > > > >
> > > > > Note that the page table does not yet scale with the memory size, and
> > > > > the secondary hash is not yet used.
> > > > >
> > > > > v5:
> > > > > Move HTABORG mask to header.
> > > > > Keep is_ppc64() for now, suggested by Alex.
> > > > >
> > > > > Merge CONFIG_PPC_64BITSUPPORT handling here so that it behaves as intended,
> > > > > not recognizing a ppc64 CPU in pure 32-bit mode.
> > > > > Stub out hash_page_32() for ppc64 to avoid having to care about it for ppc64.
> > > > >
> > > > > hash_page_64() is less critical for pure ppc build since the code needs to be
> > > > > 32-bit compatible anyway.
> > > > >
> > > > > v4:
> > > > > Don't redefine hash_page_{32,64} but add TODO comments for renaming.
> > > > > Keep and simplify hash_page().
> > > > >
> > > > > v3:
> > > > > Fix shifts. Thanks to Segher and Alex.
> > > > > Fix HTABSIZE for 32-bit ppc64: ((2 << 15) - 1) >> 16 happened to be 0,
> > > > >
> > > > > so that it worked as expected. Avoid underflow, spotted by Alex.
> > > > > Make this fix available for legacy 64-bit support as well.
> > > > > Drop get_hash_size(). It was only used in hash_page_32() and caused
> > > > >
> > > > > unnecessary conversions between HTABMASK and size.
> > > > > Introduce {mfsdr1,mtsdr1} as suggested by Alex.
> > > > >
> > > > > Note that this changes the volatility for hash_page_64().
> > > > > Conditionalize 64-bit support, drop 32-bit support for ppc64.
> > > > > Adjust hash size for ppc64.
> > > > >
> > > > > v2:
> > > > > Use HTABSIZE for ppc64, fix HTABMASK usage for ppc.
> > > > >
> > > > > Error spotted by Segher.
> > > > >
> > > > > Cc: Alexander Graf 
> > > > > Cc: Segher Boessenkool 
> > > > > Signed-off-by: Andreas Färber
> > > >
> > > > This one looks a lot cleaner. I don't see any obvious glitches either, so:
> > > >
> > > > Signed-off-by: Alexander Graf
> > >
> > > Phew... thanks! Applied as r967.
> > >
> > > A rather unproductive weekend unfortunately, no progress in gdb yet...
> >
> > Sorry for making you go through so many iterations :).
>
> I'll be stricter in the future, you too have commit rights, to create the inline functions you like. :)

True, just push me back when I get too annoying :).
> > One thing came to mind still: Please make sure that you take into account that the page table size is different for ppc64-on-ppc32 than the define inclines. If anything needs to know the real htab size, you need to have that one call a function that find it out for real.
>
> You mean the 15 that our MAX() turns into 18? Our code always reads HTABORG and HTABMASK/HTABSIZE from SDR1 so we should be just fine once it's correctly initialized.
> As noted, we should scale the page table with RAM size and use these defines as lower limit only.

I don't think it's necessary to scale with RAM size, as we shouldn't use more RAM with increasing RAM size. We're fortunately not a full blown OS :).

Alex

[Segher Boessenkool]

> I'm differentiating based on __powerpc64__ but that should probably be
> based on PVR instead? If so, then how?

I think you want to keep it compile-time; the differences between
64-bit and 32-bit are huge.  __powerpc64__ will do fine I think,
but maybe you want some CONFIG_* define.  Dunno.

Segher