[PATCH v4 12/12] RISC-V: fix auipc-jalr addresses in patched alternatives

[Conor.Dooley at microchip.com]

On 07/12/2022 22:37, Heiko Stuebner wrote:
> Am Mittwoch, 7. Dezember 2022, 21:48:08 CET schrieb Conor Dooley:
>> On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote:
>>> From: Heiko Stuebner <heiko.stuebner at vrull.eu>

>>> +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len,
>>> +                                 int patch_offset)
>>> +{
>>> +   int num_instr = len / sizeof(u32);
>>
>> instr...
>>
>>> +   int i;
>>> +
>>> +   /*
>>> +    * stop one instruction before the end, as we're checking
>>> +    * for auipc + jalr
>>> +    */
>>> +   for (i = 0; i < num_instr; i++) {
>>> +           u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32));
>>
>> ...inst...
>>
>>> +
>>> +           /* may be the start of an auipc + jalr pair */
>>> +           if (riscv_insn_is_auipc(inst) && i < num_instr - 1) {
>>
>> ...insn.
>>
>> Is there a reason for that?
> 
> I guess more of a generational issue - with the code spanning
> too much time :-)
> 
> So poll question, what would be preferred?
> I think I remember seeing all of them somewhere, so I'm unsure what
> to standardize on.

I'd vote for insn, since it's used in the functions you're calling.

>> Also, I've gotten myself slightly confused about the loop. You "stop one
>> instruction before the end" but the main loop goes from 0 -> num_instr.
>> The inner loop then checks for i < num_instr - 1. What am I missing that
>> prevents the outer loop from stopping at num_instr - 1 instead?
> 
> The idea with this is to allow a
>          if(riscv_insn_is_jal(inst))
>                  riscv_alternative_fix_jal(...)
> 
> etc, and everything else is a single instruction so needs one more
> loop iteration, only for auipc+jalr do we want to stop one earlier.
> 
> So to get this alternatives_fix_offsets() main entry point I
> made the loop do the one iteration more again.

Right, fair enough in my book :)

Thanks,
Conor.