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

[Benjamin Gray <bgray@linux.ibm.com>]

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.

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