Hi Finn,
Am 28.04.2024 um 15:28 schrieb Finn Thain:
On Sun, 28 Apr 2024, Michael Schmitz wrote:
In my tests, the fault seen there had been caused by the movesl in the
section above, hence the fixup at 20b.
Well, that was my point. A fault at MOVE.W should get fixed up at 20b, not
50b. So the patch is wrong, isn't it?
You are correct there - the fault may have happened while there still
were longwords to transfer, even though the fault PC is after the loop.
The residual from the loop must be taken into account. I'll fix that.
Should we put a NOP here to avoid having the unknown next instruction
(label 9) in the exception table? We can't actually fix up a fault
there unless by chance it was the MOVES that caused it.
... As to the unknown instructions following the final exception label:
These functions, though they contain inline assembly code are not
further inlined, and the instructions following the inline assembly are
simple boilerplate register restore stack saves that ought not to fault
(an invalid stack pointer would have faulted on function entry, if at
all).
But that's just luck. IMHO, the asm is a foot-gun even if it has not gone
off yet.
If the compiler were to inline the entire function, that could happen,
yes. So for that reason, the NOP would be safer.
On balance, I am confident the code is correct as-is. You (and in
particular, Geert) may argue though that the NOP approach follows the
principle of least surprises, and can be considered safe to apply
without further testing on 68060 and Coldfire.
If you're saying that your patch addresses a different bug, fair enough.
Nah, I was just speculating that instead of bloating the exception table
(and risking to miss a bad kernel address as source of the transfer),
addition of NOPs might be considered the safer option.
All I'm saying is that, since you're adding the NOP anyway, you could make
better use of it.
I already am, in __generic_copy_to_user - there is no exception table
entry for the jump label at the end, so no loop.
Since I have to fix patch 1 anyway, I'll add the NOP at the end of
__clear_user and have the exception table end with that NOP.
Anyway, like you, I am keen to hear from others regarding the API issue.
Too true ...
Cheers,
Michael
