Re: [PATCH 1/6] powerpc/code-patching: Implement generic text patching function

On Tue, 2022-09-20 at 05:44 +0000, Christophe Leroy wrote:

As far as I know, cachelines are minimum 64 bytes on PPC64 aren't
they ?

In practice maybe. I don't know what the convention is in the kernel in
cases where the ISA is less specific than what the supported machines
do.

quoted

Related to EA based flushing, data patching ought to run 'dcbst' on
the
'exec_addr' too. So given the icache flush only needs to be applied
to
instruction patching, and data flush only to data patching, I plan
to
move those exec_addr syncs outside of __patch_text to the relevant
public instruction/data specific entry points.

Why should it run 'dcbst' on the 'exec_addr' at all ? We have not 
written anything there.

Anyway, powerpc handles cachelines by physical address, so no matter 
which EA you use as far as it is done.

And dcbst is handled as a write by the MMU, so you just can't apply
it 
on the read-only exec address.

I was talking with Michael Ellerman and he said that, for instructions
at least, the cache operations apply to the EA. So the exec address and
the text poke address are not interchangeable. Flushing the icache
needs to be done on the exec address, not the text poke address.

The ISA uses identical wording for icache and dcache blocks ("block
containing the byte addressed by EA"), so I assume the same applies for
data too. I am trying to flush a cached value for the data EA to ensure
that the value in main memory from the text-poke EA is correctly loaded
(that's the goal, I guess that was the wrong instruction).

Or given that multiple processes sharing a physical page for RW data is
a common scenario, it could just be expected that the hardware handles
virtual address aliases for data cache.

So I don't know, and the ISA doesn't look any different to me. I'll
need some kind of confirmation either way on this.

Today raw_patch_instruction() is :

c0017ebc <raw_patch_instruction>:
c0017ebc:       90 83 00 00     stw     r4,0(r3)
c0017ec0:       7c 00 18 6c     dcbst   0,r3
c0017ec4:       7c 00 04 ac     hwsync
c0017ec8:       7c 00 1f ac     icbi    0,r3
c0017ecc:       7c 00 04 ac     hwsync
c0017ed0:       4c 00 01 2c     isync
c0017ed4:       38 60 00 00     li      r3,0
c0017ed8:       4e 80 00 20     blr
c0017edc:       38 60 ff ff     li      r3,-1
c0017ee0:       4e 80 00 20     blr

Here r4 is the value to be written.


With your patch, extract from __patch_text() is:

[...]

So as you can see, r4 now is a memory pointer and the data has to be 
loaded from there.

For this version of raw_patch_instruction

int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
{
	int err;

	if (!ppc_inst_prefixed(instr)) {
		u32 val = ppc_inst_val(instr);
		err = __patch_text(addr, &val, sizeof(val));
	} else {
		u64 val = ppc_inst_as_ulong(instr);
		err = __patch_text(addr, &val, sizeof(val));
	}

	icbi(addr);
	mb(); /* sync */
	isync();

	return err;
}

This is the 32-bit machine code my compiler generated

c0040994 <raw_patch_instruction>:
c0040994:   7c 69 1b 78     mr      r9,r3
c0040998:   90 83 00 00     stw     r4,0(r3)
c004099c:   7c 00 18 6c     dcbst   0,r3
c00409a0:   7c 00 04 ac     hwsync
c00409a4:   38 60 00 00     li      r3,0
c00409a8:   7c 00 4f ac     icbi    0,r9
c00409ac:   7c 00 04 ac     hwsync
c00409b0:   4c 00 01 2c     isync
c00409b4:   4e 80 00 20     blr
c00409b8:   38 60 ff ff     li      r3,-1
c00409bc:   4b ff ff ec     b       c00409a8
                                      <raw_patch_instruction+0x14>

It seems GCC is able to use the register automatically. But I agree
that the __patch_text generation is better, and GCC can automatically
elide the pointer stuff, so will change for v2.