[PATCH] powerpc/64: Drop ppc_inst_as_str()

[PATCH] powerpc/64: Drop ppc_inst_as_str()

[Michael Ellerman]

The ppc_inst_as_str() macro tries to make printing variable length,
aka "prefixed", instructions convenient. It mostly succeeds, but it does
hide an on-stack buffer, which triggers stack protector.

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:

  arch/powerpc/kernel/trace/ftrace.c: In function '__ftrace_modify_call':
  ./include/linux/printk.h:475:44: error: using a dangling pointer to '__str' [-Werror=dangling-pointer=]
    475 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
    ...
  arch/powerpc/kernel/trace/ftrace.c:567:17: note: in expansion of macro 'pr_err'
    567 |                 pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
        |                 ^~~~~~
  ./arch/powerpc/include/asm/inst.h:156:14: note: '__str' declared here
    156 |         char __str[PPC_INST_STR_LEN];   \
        |              ^~~~~

This could be fixed by having the caller declare the buffer, but in some
places there'd need to be two buffers. In all cases where
ppc_inst_as_str() is used the output is not really meant for user
consumption, it's almost always indicative of a kernel bug.

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.

Old:
  c000000000111170  60420000      ori     r2,r2,0
  c000000000111174  04100001 e580fb00     .long 0xe580fb0004100001

New:
  c00000000010f90c  60420000      ori     r2,r2,0
  c00000000010f910  e580fb0004100001      .long 0xe580fb0004100001

Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Reported-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/inst.h      | 19 -------------------
 arch/powerpc/kernel/kprobes.c        |  2 +-
 arch/powerpc/kernel/trace/ftrace.c   | 24 +++++++++++++-----------
 arch/powerpc/lib/test_emulate_step.c |  6 +++---
 arch/powerpc/xmon/xmon.c             |  2 +-
 5 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index b49aae9f6f27..684d3f453282 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -139,25 +139,6 @@ static inline void ppc_inst_write(u32 *ptr, ppc_inst_t x)
 		*(u64 *)ptr = ppc_inst_as_ulong(x);
 }
 
-#define PPC_INST_STR_LEN sizeof("00000000 00000000")
-
-static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], ppc_inst_t x)
-{
-	if (ppc_inst_prefixed(x))
-		sprintf(str, "%08x %08x", ppc_inst_val(x), ppc_inst_suffix(x));
-	else
-		sprintf(str, "%08x", ppc_inst_val(x));
-
-	return str;
-}
-
-#define ppc_inst_as_str(x)		\
-({					\
-	char __str[PPC_INST_STR_LEN];	\
-	__ppc_inst_as_str(__str, x);	\
-	__str;				\
-})
-
 static inline int __copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
 {
 	unsigned int val, suffix;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 1c97c0f177ae..912d4f8a13be 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -269,7 +269,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
 		 * So, we should never get here... but, its still
 		 * good to catch them, just in case...
 		 */
-		printk("Can't step on instruction %s\n", ppc_inst_as_str(insn));
+		printk("Can't step on instruction %08lx\n", ppc_inst_as_ulong(insn));
 		BUG();
 	} else {
 		/*
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 2a893e06e4f1..cab67b5120b9 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -69,8 +69,8 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
 
 	/* Make sure it is what we expect it to be */
 	if (!ppc_inst_equal(replaced, old)) {
-		pr_err("%p: replaced (%s) != old (%s)",
-		(void *)ip, ppc_inst_as_str(replaced), ppc_inst_as_str(old));
+		pr_err("%p: replaced (%08lx) != old (%08lx)", (void *)ip,
+		       ppc_inst_as_ulong(replaced), ppc_inst_as_ulong(old));
 		return -EINVAL;
 	}
 
@@ -127,7 +127,7 @@ __ftrace_make_nop(struct module *mod,
 
 	/* Make sure that that this is still a 24bit jump */
 	if (!is_bl_op(op)) {
-		pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
+		pr_err("Not expected bl: opcode is %08lx\n", ppc_inst_as_ulong(op));
 		return -EINVAL;
 	}
 
@@ -159,8 +159,8 @@ __ftrace_make_nop(struct module *mod,
 		/* We expect either a mflr r0, or a std r0, LRSAVE(r1) */
 		if (!ppc_inst_equal(op, ppc_inst(PPC_RAW_MFLR(_R0))) &&
 		    !ppc_inst_equal(op, ppc_inst(PPC_INST_STD_LR))) {
-			pr_err("Unexpected instruction %s around bl _mcount\n",
-			       ppc_inst_as_str(op));
+			pr_err("Unexpected instruction %08lx around bl _mcount\n",
+			       ppc_inst_as_ulong(op));
 			return -EINVAL;
 		}
 	} else if (IS_ENABLED(CONFIG_PPC64)) {
@@ -174,7 +174,8 @@ __ftrace_make_nop(struct module *mod,
 		}
 
 		if (!ppc_inst_equal(op,  ppc_inst(PPC_INST_LD_TOC))) {
-			pr_err("Expected %08lx found %s\n", PPC_INST_LD_TOC, ppc_inst_as_str(op));
+			pr_err("Expected %08lx found %08lx\n", PPC_INST_LD_TOC,
+			       ppc_inst_as_ulong(op));
 			return -EINVAL;
 		}
 	}
@@ -312,7 +313,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
 
 	/* Make sure that that this is still a 24bit jump */
 	if (!is_bl_op(op)) {
-		pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
+		pr_err("Not expected bl: opcode is %08lx\n", ppc_inst_as_ulong(op));
 		return -EINVAL;
 	}
 
@@ -416,8 +417,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		return -EFAULT;
 
 	if (!expected_nop_sequence(ip, op[0], op[1])) {
-		pr_err("Unexpected call sequence at %p: %s %s\n",
-		ip, ppc_inst_as_str(op[0]), ppc_inst_as_str(op[1]));
+		pr_err("Unexpected call sequence at %p: %08lx %08lx\n", ip,
+		       ppc_inst_as_ulong(op[0]), ppc_inst_as_ulong(op[1]));
 		return -EINVAL;
 	}
 
@@ -486,7 +487,8 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
 	}
 
 	if (!ppc_inst_equal(op, ppc_inst(PPC_RAW_NOP()))) {
-		pr_err("Unexpected call sequence at %p: %s\n", ip, ppc_inst_as_str(op));
+		pr_err("Unexpected call sequence at %p: %08lx\n",
+		       ip, ppc_inst_as_ulong(op));
 		return -EINVAL;
 	}
 
@@ -564,7 +566,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 
 	/* Make sure that that this is still a 24bit jump */
 	if (!is_bl_op(op)) {
-		pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
+		pr_err("Not expected bl: opcode is %08lx\n", ppc_inst_as_ulong(op));
 		return -EINVAL;
 	}
 
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 4f141daafcff..f2e47be05e8c 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -1616,11 +1616,11 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
 	if (analysed != 1 || GETTYPE(op.type) != COMPUTE) {
 		if (negative)
 			return -EFAULT;
-		pr_info("emulation failed, instruction = %s\n", ppc_inst_as_str(instr));
+		pr_info("emulation failed, instruction = %08lx\n", ppc_inst_as_ulong(instr));
 		return -EFAULT;
 	}
 	if (analysed == 1 && negative)
-		pr_info("negative test failed, instruction = %s\n", ppc_inst_as_str(instr));
+		pr_info("negative test failed, instruction = %08lx\n", ppc_inst_as_ulong(instr));
 	if (!negative)
 		emulate_update_regs(regs, &op);
 	return 0;
@@ -1637,7 +1637,7 @@ static int __init execute_compute_instr(struct pt_regs *regs,
 	/* Patch the NOP with the actual instruction */
 	patch_instruction_site(&patch__exec_instr, instr);
 	if (exec_instr(regs)) {
-		pr_info("execution failed, instruction = %s\n", ppc_inst_as_str(instr));
+		pr_info("execution failed, instruction = %08lx\n", ppc_inst_as_ulong(instr));
 		return -EFAULT;
 	}
 
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index fff81c2300fa..6043454842cb 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3048,7 +3048,7 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
 		dotted = 0;
 		last_inst = inst;
 		if (praddr)
-			printf(REG"  %s", adr, ppc_inst_as_str(inst));
+			printf(REG"  %08lx", adr, ppc_inst_as_ulong(inst));
 		printf("\t");
 		if (!ppc_inst_prefixed(inst))
 			dump_func(ppc_inst_val(inst), adr);
-- 
2.35.3

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

[Segher Boessenkool]

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).

> 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.

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);
}

or something like that.


Segher

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

[Bagas Sanjaya]

Hi,

On Tue, May 31, 2022 at 04:59:36PM +1000, Michael Ellerman wrote:
> The ppc_inst_as_str() macro tries to make printing variable length,
> aka "prefixed", instructions convenient. It mostly succeeds, but it does
> hide an on-stack buffer, which triggers stack protector.
> 
> 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:
> 
>   arch/powerpc/kernel/trace/ftrace.c: In function '__ftrace_modify_call':
>   ./include/linux/printk.h:475:44: error: using a dangling pointer to '__str' [-Werror=dangling-pointer=]
>     475 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
>     ...
>   arch/powerpc/kernel/trace/ftrace.c:567:17: note: in expansion of macro 'pr_err'
>     567 |                 pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
>         |                 ^~~~~~
>   ./arch/powerpc/include/asm/inst.h:156:14: note: '__str' declared here
>     156 |         char __str[PPC_INST_STR_LEN];   \
>         |              ^~~~~
> 
> This could be fixed by having the caller declare the buffer, but in some
> places there'd need to be two buffers. In all cases where
> ppc_inst_as_str() is used the output is not really meant for user
> consumption, it's almost always indicative of a kernel bug.
> 
> 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.
> 
> Old:
>   c000000000111170  60420000      ori     r2,r2,0
>   c000000000111174  04100001 e580fb00     .long 0xe580fb0004100001
> 
> New:
>   c00000000010f90c  60420000      ori     r2,r2,0
>   c00000000010f910  e580fb0004100001      .long 0xe580fb0004100001
> 
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Reported-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

The arch/powerpc/kernel/trace/ftrace.c builds successfully, however
there is also other build error for which I have reported at [1].

Thanks.

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

[1]: https://lore.kernel.org/linuxppc-dev/YpbUcPrm61RLIiZF@debian.me/

-- 
An old man doll... just what I always wanted! - Clara

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

[Bagas Sanjaya]

On 6/1/22 10:03, Bagas Sanjaya wrote:
>>
>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> Reported-by: Petr Mladek <pmladek@suse.com>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> 
> The arch/powerpc/kernel/trace/ftrace.c builds successfully, however
> there is also other build error for which I have reported at [1].
> 
> Thanks.
> 
> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
> 
> [1]: https://lore.kernel.org/linuxppc-dev/YpbUcPrm61RLIiZF@debian.me/
> 

Oops, I forget to mention: is this patch meant for backporting into
stable tree? I don't see Fixes: tag and Cc: stable@vger.kernel.org
in the patch.

-- 
An old man doll... just what I always wanted! - Clara

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

[Michael Ellerman]

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.

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.

>> 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.

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.

> 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.

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.

cheers

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

[Segher Boessenkool]

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.


Segher

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

[Jordan Niethe]

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

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

[Segher Boessenkool]

On Thu, Jun 02, 2022 at 01:01:04PM +1000, Jordan Niethe wrote:
> > 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.

You cannot do such a (possibly) unaligned access from C though, not
without invoking undefined behaviour.  The compiler usually lets you get
away with it, but there are no guarantees.  You can make sure you only
ever do such an access from assembler code of course.

Swapping the two halves of a register costs at most one insn.  It is
harmful premature optimisation to make this single cycle advantage
override more important consideration (almost everything else :-) )


Segher

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

[Jordan Niethe]

On Thu, Jun 2, 2022 at 6:49 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Jun 02, 2022 at 01:01:04PM +1000, Jordan Niethe wrote:
> > > 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.
>
> You cannot do such a (possibly) unaligned access from C though, not
> without invoking undefined behaviour.  The compiler usually lets you get
> away with it, but there are no guarantees.  You can make sure you only
> ever do such an access from assembler code of course.

Would using inline assembly to do it be ok?

>
> Swapping the two halves of a register costs at most one insn.  It is
> harmful premature optimisation to make this single cycle advantage
> override more important consideration (almost everything else :-) )

I'm not sure I follow. We are not doing this as an optimisation, but
out of the necessity of writing
the prefixed instruction to memory in a single instruction so that we
don't end up with half an
instruction in the kernel image.

>
>
> Segher

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

[Segher Boessenkool]

Hi!

On Fri, Jun 03, 2022 at 03:03:05PM +1000, Jordan Niethe wrote:
> On Thu, Jun 2, 2022 at 6:49 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Thu, Jun 02, 2022 at 01:01:04PM +1000, Jordan Niethe wrote:
> > > > 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.
> >
> > You cannot do such a (possibly) unaligned access from C though, not
> > without invoking undefined behaviour.  The compiler usually lets you get
> > away with it, but there are no guarantees.  You can make sure you only
> > ever do such an access from assembler code of course.
> 
> Would using inline assembly to do it be ok?

You cannot refer to the instruction words as one 8-byte integer in
memory (with "m" or similar), since such a thing is not valid C.  You
can input the address of it to the inline asm of course, and use a
clobber "memory", or also give it the actual memory as input, but as
bytes or words or such, with something like
  "m"(*(const u32 (*)[2]) ptr)

> > Swapping the two halves of a register costs at most one insn.  It is
> > harmful premature optimisation to make this single cycle advantage
> > override more important consideration (almost everything else :-) )
> 
> I'm not sure I follow. We are not doing this as an optimisation, but
> out of the necessity of writing
> the prefixed instruction to memory in a single instruction so that we
> don't end up with half an
> instruction in the kernel image.

Ah.  That does not necessitate having this different for LE at all
though!  The function that does the patching has to do it as one
atomic memory access (which an ld only is because prefixed insns cannot
cross 64-byte address boundaries btw), but that does not mean the kernel
has to use the packing into one u64 it needs for that anywhere else,
certainly not if that just complicates matters!


Segher

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

[Michael Ellerman]

On Tue, 31 May 2022 16:59:36 +1000, Michael Ellerman wrote:
> The ppc_inst_as_str() macro tries to make printing variable length,
> aka "prefixed", instructions convenient. It mostly succeeds, but it does
> hide an on-stack buffer, which triggers stack protector.
> 
> 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:
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/64: Drop ppc_inst_as_str()
      https://git.kernel.org/powerpc/c/2a83afe72a2b5760155c2dd840c776aee292dc90

cheers