[OpenBIOS] [PATCH v4] ppc: SDR1 fixes
Andreas Färber
andreas.faerber at web.de
Sun Nov 21 22:40:52 CET 2010
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 <agraf at suse.de>
>> Cc: Segher Boessenkool <segher at kernel.crashing.org>
>> Signed-off-by: Andreas Färber <andreas.faerber at web.de>
>> ---
>> 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
More information about the OpenBIOS
mailing list