[UPDATED] [PATCH v4 3/7] [ppc] Process dynamic relocations for kernel

[Segher Boessenkool]

Hi Suzuki,

Looks quite good, a few comments...

> +get_type:
> +	/* r4 holds the relocation type */
> +	extrwi	r4, r4, 8, 24	/* r4 = ((char*)r4)[3] */

This comment is confusing (only makes sense together with the
lwz a long way up).

> +nxtrela:
> +	/*
> +	 * We have to flush the modified instructions to the
> +	 * main storage from the d-cache. And also, invalidate the
> +	 * cached instructions in i-cache which has been modified.
> +	 *
> +	 * We delay the msync / isync operation till the end, since
> +	 * we won't be executing the modified instructions until
> +	 * we return from here.
> +	 */
> +	dcbst	r4,r7
> +	icbi	r4,r7

You still need a sync between these two.  Without it, the icbi can
complete before the dcbst for the same address does, which leaves
room for an instruction fetch from that address to get old data.

> +	cmpwi	r8, 0		/* relasz = 0 ? */
> +	ble	done
> +	add	r9, r9, r6	/* move to next entry in the .rela table */
> +	subf	r8, r6, r8	/* relasz -= relaent */
> +	b	applyrela
> +
> +done:
> +	msync			/* Wait for the flush to finish */

The instruction is called "sync".  msync is a BookE thing.

>  	next if (/R_PPC64_RELATIVE/ or /R_PPC64_NONE/ or
>  	         /R_PPC64_ADDR64\s+mach_/);
> +	next if (/R_PPC_ADDR16_LO/ or /R_PPC_ADDR16_HI/ or
> +		 /R_PPC_ADDR16_HA/ or /R_PPC_RELATIVE/);

Nothing new, but these should probably have \b or \s or just
a space on each side.


Segher