On Wed, 2022-11-02 at 11:13 +0100, Christophe Leroy wrote: > Le 02/11/2022 à 10:43, Christophe Leroy a écrit : > > Le 25/10/2022 à 06:44, Benjamin Gray a écrit : > > > Verifies that if the instruction patching did not return an error > > > then > > > the value stored at the given address to patch is now equal to > > > the > > > instruction we patched it to. > > > > Why do we need that verification ? Until now it wasn't necessary, > > can > > you describe a failure that occurs because we don't have this > > verification ? Or is that until now it was reliable but the new > > method > > you are adding will not be as reliable as before ? > > > > What worries me with that new verification is that you are reading > > a > > memory address with is mostly only used for code execution. That > > means: > > - You will almost always take a DATA TLB Miss, hence performance > > impact. > > - If one day we want Exec-only text mappings, it will become > > problematic. > > > > So really the question is, is that patch really required ? > > > > > > > > Signed-off-by: Benjamin Gray <bg...@linux.ibm.com> > > > --- > > > arch/powerpc/lib/code-patching.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/powerpc/lib/code-patching.c > > > b/arch/powerpc/lib/code-patching.c > > > index 3b3b09d5d2e1..b0a12b2d5a9b 100644 > > > --- a/arch/powerpc/lib/code-patching.c > > > +++ b/arch/powerpc/lib/code-patching.c > > > @@ -192,6 +192,8 @@ static int do_patch_instruction(u32 *addr, > > > ppc_inst_t instr) > > > err = __do_patch_instruction(addr, instr); > > > local_irq_restore(flags); > > > + WARN_ON(!err && !ppc_inst_equal(instr, > > > ppc_inst_read(addr))); > > > + > > Another point: you are doing the check outside of IRQ disabled > section, > is that correct ? What if an interrupt changed it in-between ? > > Or insn't that possible ? In that case what's the real purpose of > disabling IRQs here ?
Disabling IRQ's also serves to prevent the writeable mapping being visible outside of the patching function from my understanding. But I would move the check inside the disabled section if I were keeping it.