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

[Benjamin Gray]

On Mon, 2022-09-19 at 07:16 +0000, Christophe Leroy wrote:
> Why would it be unpredictable ? Only one page is mapped. If it
> crosses 
> the boundary, __put_kernel_nofault() will fail in a controled manner.
> I see no point in doing the check before every write.

Oh I didn't see that get_vm_area automatically adds a guard. You're
right then, it's redundant. I had assumed there could be a writeable
mapping directly after.

> And while you are thinking about alignment, don't forget that dcbst
> and 
> icbi apply on a give cacheline. If your memory crosses a cacheline
> you 
> may have a problem.

Yeah, though this applies to the existing patch_instruction too (in
theory; prefixed instructions cannot cross a 64 byte boundary, but the
ISA does not specify minimum cacheline size). I don't have a nice
solution right now though. The flush needs to be done on the effective
address (i.e. text poke address) according to the ISA, but the text
poke address is only valid within the IRQ save region. So non-prefixed
instruction patching would either pay for some kind of check, or need
special casing.

Maybe an "is aligned" flag in a generic __patch_text to make the extra
flush conditional is good enough?


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.

> > > > +               case 8:
> > > > +                       __put_kernel_nofault(dest, src, u64,
> > > > failed);
> > > > +                       break;
> > > 
> > > Is case 8 needed for PPC32 ?
> > 
> > I don't have a particular need for it, but the underlying
> > __put_kernel_nofault is capable of it so I included it.
> 
> Well, not including it will allow you to pass the source as a 'long'
> as 
> mentionned above.

I checked the machine code of a 32 bit build, but it still passes the
pointer in a register. I also checked all 3 ABI docs and they say a
pointer is the same size as a long. Could you clarify when a pointer is
passed on the stack but not a long?

Or do you mean that we could pass the pointed-to data in a register and
skip the pointer altogether? That seems like a good choice, but
__put_kernel_nofault takes a pointer to the source and the
implementation is very complex. I don't think I can safely write the
modified version we'd need at this point.