Re: [PATCH] powerpc: fixup lwsync at runtime

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4013 bytes --]

On Tue, 2008-07-01 at 00:32 -0500, Kumar Gala wrote:
> To allow for a single kernel image on e500 v1/v2/mc we need to fixup lwsync
> at runtime.  On e500v1/v2 lwsync causes an illop so we need to patch up
> the code.  We default to 'sync' since that is always safe and if the cpu
> is capable we will replace 'sync' with 'lwsync'.
> 
> We introduce CPU_FTR_LWSYNC as a way to determine at runtime if this is
> needed.  This flag could be moved elsewhere since we dont really use it
> for the normal CPU_FTR purpose.
> 
> Finally we only store the relative offset in the fixup section to keep it
> as small as possible rather than using a full fixup_entry.

How many entries are we talking? I guess it's not much bother to have a
separate section.

> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> index 9e83add..0109e7f 100644
> --- a/arch/powerpc/kernel/setup_32.c
> +++ b/arch/powerpc/kernel/setup_32.c
> @@ -101,6 +101,10 @@ unsigned long __init early_init(unsigned long dt_ptr)
>  			  PTRRELOC(&__start___ftr_fixup),
>  			  PTRRELOC(&__stop___ftr_fixup));
> 
> +	do_lwsync_fixups(spec->cpu_features,
> +			 PTRRELOC(&__start___lwsync_fixup),
> +			 PTRRELOC(&__stop___lwsync_fixup));
> +

This could be changed to use cur_cpu_spec->cpu_features, and then all
the call sites would be passing that, which would mean
do_lwsync_fixups() could just check cur_cpu_spec->cpu_features directly.

> diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
> index 48e1ed8..ac5f5a1 100644
> --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -110,6 +110,22 @@ void do_feature_fixups(unsigned long value, void *fixup_start, void *fixup_end)
>  	}
>  }
> 
> +void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
> +{
> +	unsigned int *start, *end, *dest;
> +
> +	if (!(value & CPU_FTR_LWSYNC))
> +		return ;
> +
> +	start = fixup_start;
> +	end = fixup_end;
> +
> +	for (; start < end; start++) {
> +		dest = (void *)start + *start;
> +		patch_instruction(dest, PPC_LWSYNC_INSTR);
> +	}
> +}
> +
>  #ifdef CONFIG_FTR_FIXUP_SELFTEST

...

No tests? :)

> diff --git a/include/asm-powerpc/synch.h b/include/asm-powerpc/synch.h
> index 42a1ef5..4737c61 100644
> --- a/include/asm-powerpc/synch.h
> +++ b/include/asm-powerpc/synch.h
> @@ -3,20 +3,37 @@
>  #ifdef __KERNEL__
> 
>  #include <linux/stringify.h>
> +#include <asm/feature-fixups.h>
> 
> -#if defined(__powerpc64__) || defined(CONFIG_PPC_E500MC)
> -#define __SUBARCH_HAS_LWSYNC
> -#endif
> +#ifndef __ASSEMBLY__
> +extern unsigned int __start___lwsync_fixup, __stop___lwsync_fixup;
> +extern void do_lwsync_fixups(unsigned long value, void *fixup_start,
> +			     void *fixup_end);
> +#endif /* __ASSEMBLY__ */
> +
> +#define START_LWSYNC_SECTION(label)	label##1:
> +#define MAKE_LWSYNC_SECTION_ENTRY(label, sect)		\
> +label##4:						\
> +	.pushsection sect,"a";				\
> +	.align 2;					\
> +label##5:					       	\
> +	.long label##1b-label##5b;			\
> +	.popsection;

I'd rather this was in feature-fixups.h, seeing as I just moved all the
feature-fixup related macros in there :)

It might make more sense to use label##2 and label##3.

> 
> -#ifdef __SUBARCH_HAS_LWSYNC
> +#if defined(__powerpc64__)
>  #    define LWSYNC	lwsync
> +#elif defined(CONFIG_E500)
> +#    define LWSYNC					\
> +	START_LWSYNC_SECTION(97);			\
> +	sync;						\
> +	MAKE_LWSYNC_SECTION_ENTRY(97, __lwsync_fixup);
>  #else
>  #    define LWSYNC	sync
>  #endif

Using 97 means you can't nest an LWSYNC inside a feature or firmware
feature section, if you use say 96 then nesting should work.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]