Re: [PATCH v8 11/30] powerpc: Use a datatype for instructions

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 08/05/2020 à 03:51, Jordan Niethe a écrit :
> On Wed, May 6, 2020 at 1:45 PM Jordan Niethe <jniethe5@gmail.com> wrote:
>>
>> Currently unsigned ints are used to represent instructions on powerpc.
>> This has worked well as instructions have always been 4 byte words.
>> However, a future ISA version will introduce some changes to
>> instructions that mean this scheme will no longer work as well. This
>> change is Prefixed Instructions. A prefixed instruction is made up of a
>> word prefix followed by a word suffix to make an 8 byte double word
>> instruction. No matter the endianness of the system the prefix always
>> comes first. Prefixed instructions are only planned for powerpc64.
>>
>> Introduce a ppc_inst type to represent both prefixed and word
>> instructions on powerpc64 while keeping it possible to exclusively have
>> word instructions on powerpc32.
>>
>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> ---
>> v4: New to series
>> v5: Add to epapr_paravirt.c, kgdb.c
>> v6: - setup_32.c: machine_init(): Use type
>>      - feature-fixups.c: do_final_fixups(): Use type
>>      - optprobes.c: arch_prepare_optimized_kprobe(): change a void * to
>>        struct ppc_inst *
>>      - fault.c: store_updates_sp(): Use type
>>      - Change ppc_inst_equal() implementation from memcpy()
>> v7: - Fix compilation issue in early_init_dt_scan_epapr() and
>>        do_patch_instruction() with CONFIG_STRICT_KERNEL_RWX
>> v8: - style
>>      - Use in crash_dump.c, mpc86xx_smp.c, smp.c
>> ---

[...]

>>
> 
> Hi mpe,
> Could you add this fixup.
> --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -356,7 +356,7 @@ static void patch_btb_flush_section(long *curr)
>          end = (void *)curr + *(curr + 1);
>          for (; start < end; start++) {
>                  pr_devel("patching dest %lx\n", (unsigned long)start);
> -               patch_instruction(start, ppc_inst(PPC_INST_NOP));
> +               patch_instruction((struct ppc_inst *)start,
> ppc_inst(PPC_INST_NOP));
>          }
>   }
> 

Why not declare stard and end as struct ppc_inst ? Wouldn't it be 
cleaner than a cast ?

Christophe