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

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

On 12/11/11 01:32, Segher Boessenkool wrote:
> 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).

Agree, will fix them.
>
>> +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.
>
Ok.
>> + 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.
Will fix this too. Also will include the R_PPC_NONE to the list
of valid relocations.

Thanks
Suzuki


>
>
> Segher
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>