Subject: Re: [PATCH v4 14/16] powerpc64: Add prefixed instructions to instruction data type

From: Nicholas Piggin <[email-masked]>
Date: Tue, 24 Mar 2020 15:40:56 +1000

Jordan Niethe's on March 24, 2020 9:45 am:
> On Mon, Mar 23, 2020 at 6:37 PM Nicholas Piggin <[email-masked]> wrote:
>>
>> Jordan Niethe's on March 20, 2020 3:18 pm:
>> I'm a bit against using partially constructed opaque type for things
>> like this, even if it is in the code that knows about the type. We
>> could modify ppc_inst_prefixed() to assert that pad is equal to zero
>> (or some poisoned value) if it's not prefixed. Or do some validation
>> on the suffix if it is.
> Okay what about something like:
> +static inline ppc_inst ppc_inst_read(const void *ptr)
> +{
> +     u32 prefix, suffix;
> +     prefix = *(u32 *)ptr;
> +     if (prefix >> 26 == 1)
> +             suffix = *((u32 *)ptr + 1);
> +     else
> +             suffix = 0;
> +     return PPC_INST_PREFIX(prefix, suffix);
> +}

Sure, if that's the best way to test prefix.

>> Although there's proably no real performance or atomicity issues here,
>> I'd be pleased if we could do a case for prefixed and a case for non
>> prefixed, and store the non-prefixed with "std". Just for the principle
>> of not having half-written instructions in the image.
> Do you mean store the prefixed with std?

Oops, yes.

>> > @@ -881,7 +882,6 @@ static struct bpt *new_breakpoint(unsigned long a)
>> >               if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
>> >                       bp->address = a;
>> >                       bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS);
>> > -                     patch_instruction(bp->instr + 1, PPC_INST(bpinstr));
>> >                       return bp;
>> >               }
>> >       }
>>
>> Why is this okay to remove?
> When we only had word instructions the bpt was just patched in here
> once and that was that.
> With prefixed instructions bp->instr + 1 might be the suffix. So I
> moved putting the breakpoint to insert_bpts():
> patch_instruction(bp->instr + ppc_inst_len(instr), PPC_INST(bpinstr));

Ah okay.

Thanks,
Nick