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.