Re: [PATCH] powerpc/64: Drop ppc_inst_as_str()

[Jordan Niethe <jniethe5@gmail.com>]

On Thu, Jun 2, 2022 at 2:22 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Jun 01, 2022 at 08:43:01PM +1000, Michael Ellerman wrote:
> > Segher Boessenkool <segher@kernel.crashing.org> writes:
> > > Hi!
> > >
> > > On Tue, May 31, 2022 at 04:59:36PM +1000, Michael Ellerman wrote:
> > >> More problematically it doesn't compile at all with GCC 12, due to the
> > >> fact that it returns the char buffer declared inside the macro:
> > >
> > > It returns a pointer to a buffer on stack.  It is not valid C to access
> > > that buffer after the function has returned (and indeed it does not
> > > work, in general).
> >
> > It's a statement expression though, not a function. So it doesn't return
> > as such, that would be obviously wrong.
>
> Yes, wrong language, my bad.  But luckily it doesn't matter if this is a
> function or not anyway: the question is about scopes and lifetimes :-)
>
> > But I'm not a language lawyer, so presumably it's not valid to refer to
> > the variable after it's gone out of scope.
> >
> > Although we do use that same pattern in many places where the value of
> > the expression is a scalar type.
>
> It's an object with automatic storage duration.  Its lifetime ends when
> the scope is left, which is at the end of the statement expression, so
> before the object is used.
>
> The value of the expression can be used just fine, sure, but the object
> it points to has ceased to exist, so dereferencing that pointer is
> undefined behaviour.
>
> > >> A simpler solution is to just print the value as an unsigned long. For
> > >> normal instructions the output is identical. For prefixed instructions
> > >> the value is printed as a single 64-bit quantity, whereas previously the
> > >> low half was printed first. But that is good enough for debug output,
> > >> especially as prefixed instructions will be rare in practice.
> > >
> > > Prefixed insns might be somewhat rare currently, but it will not stay
> > > that way.
> >
> > These are all printing kernel instructions, not userspace. I should have
> > said that in the change log.
>
> Ah!  In that case, it will take quite a bit longer before you will see
> many prefixed insns, sure.
>
> > The kernel doesn't build for -mcpu=power10 because we haven't done any
> > changes for pcrel.
> >
> > We will do that one day, but not soon.
>
> Yeah, pcrel is the big hitter currently.  But with the extra opcode
> space we have now, maybe something else will show up that even the
> kernel will use.  I cannot predict the future very well :-)
>
> > > It is not hard to fix the problem here?  The only tricky part is that
> > > ppc_inst_as_ulong swaps the two halves for LE, for as far as I can see
> > > no reason at all :-(
> > >
> > > If it didn't it would be easy to detect prefixed insns (because they
> > > then are guaranteed to be > 0xffffffff), and it is easy to print them
> > > with a space between the two opcodes, with a utility function:
> > >
> > > void print_insn_bytes_nicely(unsigned long insn)
> > > {
> > >     if (insn > 0xffffffff)
> > >             printf("%08x ", insn >> 32);
> > >     printf("%08x", insn & 0xffffffff);
> > > }
> >
> > We don't want to do that because it can lead to interleaving messages
> > between different CPUs in the kernel log.
>
> Yuck.
>
> void print_insn_bytes_nicely(unsigned long insn)
> {
>         if (insn > 0xffffffff)
>                 printf("%08x ", insn >> 32, insn & 0xffffffff);
>         else
>                 printf("%08x", insn & 0xffffffff);
> }
>
> But it makes things much less enticing, alright.
>
> > In the medium term there's some changes to printk that might land soon
> > (printbuf), which would mean we could more easily define a custom printk
> > formatter for printing prefixed instructions.
>
> Yeah :-)
>
> What about the more fundamental thing?  Have the order of the two halves
> of a prefixed insn as ulong not depend on endianness?  It really is two
> opcodes, and the prefixed one is first, always, even in LE.
The reason would be the value of as ulong is then used to write a
prefixed instruction to
memory with std.
If both endiannesses had the halves the same one of them would store
the suffix in front of the prefix.
>
>
> Segher