Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded

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

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