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

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

On Wed, 2022-11-02 at 09:43 +0000, Christophe Leroy wrote:
> 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 ?

It's required as much as any sanity check in the kernel. I agree
running it all the time is not great though, I would prefer to make it
a debug-only check. I've seen VM_WARN_ON be used for this purpose I
think? It's especially useful now with churn on the code-patching code.
I don't expect the new method to be unreliable—I wouldn't be submitting
it if I did—but I'd much prefer to have an obvious tell if it does turn
out so.

But the above is all moot because we allow parallel patching, so the
check is just plain incorrect.