Re: [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type

[Jordan Niethe <jniethe5@gmail.com>]

On Mon, Apr 6, 2020 at 8:25 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 06/04/2020 à 11:52, Alistair Popple a écrit :
>
> [...]
>
> >> @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct
> >> ppc_inst x) return ppc_inst(swab32(ppc_inst_val(x)));
> >>   }
> >>
> >> +static inline u32 ppc_inst_val(struct ppc_inst x)
> >> +{
> >> +    return x.val;
> >> +}
> >> +
> >>   static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> >>   {
> >>      return *ptr;
> >>   }
> >>
> >> +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
> >> +{
> >> +    *ptr = x;
> >> +}
> >> +
> >> +#endif /* __powerpc64__ */
> >> +
> >>   static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
> >>   {
> >>      return !memcmp(&x, &y, sizeof(struct ppc_inst));
> >>   }
> >
> > Apologies for not picking this up earlier, I was hoping to get to the bottom
> > of the issue I was seeing before you sent out v5. However the above definition
> > of instruction equality does not seem correct because it does not consider the
> > case when an instruction is not prefixed - a non-prefixed instruction should be
> > considered equal if the first 32-bit opcode/value is the same. Something like:
> >
> >       if (ppc_inst_prefixed(x) != ppc_inst_prefixed(y))
> >               return false;
> >       else if (ppc_inst_prefixed(x))
> >               return !memcmp(&x, &y, sizeof(struct ppc_inst));
>
> Are we sure memcmp() is a good candidate for the comparison ? Can we do
> simpler ? Especially, I understood a prefixed instruction is a 64 bits
> properly aligned instruction, can we do a simple u64 compare ? Or is GCC
> intelligent enough to do that without calling memcmp() function which is
> heavy ?
As it is GCC does call memcmp(). I'll try to make it simpler.
>
> >       else
> >               return x.val == y.val;
> >
> > This was causing failures in ftrace_modify_code() as it would falsely detect
> > two non-prefixed instructions as being not equal due to differences in the suffix.
> >
>
> Christophe