Re: [PATCH v2 1/5] [ppc] Process dynamic relocations for kernel

[Suzuki Poulose <suzuki@in.ibm.com>]

On 11/03/11 05:06, Josh Poimboeuf wrote:
> On Tue, 2011-10-25 at 17:23 +0530, Suzuki K. Poulose wrote:
>> The following patch implements the dynamic relocation processing for
>> PPC32 kernel. relocate() accepts the target virtual address and relocates
>>   the kernel image to the same.
>
> Hi Suzuki,
>
> Thanks for the patches.  I've been testing them on a 440-based card, and
> encountered TLB error exceptions because the BSS section wasn't getting
> properly cleared in early_init().

Thanks a lot for the testing.
>
> It turns out that some of the instructions which were modified in
> relocate() weren't then getting flushed out of the d-cache into memory.
> After that, early_init() executed the stale (non-modified) instructions
> for the BSS area.  Those instructions just accessed offset 0 instead of
> the actual BSS-related offsets.  That resulted in BSS not getting`
> zeroed.
>
> I was able to verify this on my 440 by comparing the d-cache and i-cache
> entries for the BSS-accessing instructions in early_init() using a
> RISCWatch.  As I suspected, the instructions in the d-cache showed the
> corrected offsets, but the i-cache showed the old, non-relocated
> offsets.
>
> To fix the issue, I wrote the following patch, applied on top of your
> patches.  Suggestions and comments are welcome.
>
>
>
>  From c88ae39da0c0352f411aca8d9636990a442d47da Mon Sep 17 00:00:00 2001
> From: Josh Poimboeuf<jpoimboe@linux.vnet.ibm.com>
> Date: Wed, 2 Nov 2011 16:41:24 -0500
> Subject: [PATCH] Flush relocated instructions from data cache
>
> After updating instructions with relocated addresses, flush them from
> the data cache and invalidate the icache line so we don't execute stale
> instructions.
>
> Signed-off-by: Josh Poimboeuf<jpoimboe@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/reloc_32.S |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/kernel/reloc_32.S
> b/arch/powerpc/kernel/reloc_32.S
> index 045d61e..a92857d 100644
> --- a/arch/powerpc/kernel/reloc_32.S
> +++ b/arch/powerpc/kernel/reloc_32.S
> @@ -137,6 +137,9 @@ get_type:
>   	lwz	r0, 8(r9)	/* r_addend */
>   	add	r0, r0, r3	/* final addend */
>   	stwx	r0, r4, r7	/* memory[r4+r7]) = (u32)r0 */
> +	dcbst	r4,r7		/* flush dcache line to memory */
> +	sync			/* wait for flush to complete */
> +	icbi	r4,r7		/* invalidate icache line */

Doing it this way has two drawbacks :

1) Placing it here in relocate would do the flushing for each and every update.
2) I would like to keep this code as generic as possible for the PPC32 code.

Could we move this to the place from relocate is called and flush the d-cache and
i-cache entirely ?

Thanks

Suzuki