On 9/16/2025 8:22 PM, Sean Christopherson wrote:
On Tue, Sep 16, 2025, Paolo Bonzini wrote:
On 8/27/25 01:04, Roman Kisel wrote:
On 8/26/2025 5:07 AM, Peter Zijlstra wrote:
I do not know what OpenHCL is. Nor is it clear from the code what NMIs
can't happen. Anyway, same can be achieved with breakpoints / kprobes.
You can get a trap after setting CR2 and scribble it.
You simply cannot use CR2 this way.
The code in question runs with interrupts disabled, and the kernel runs
without the memory swapping when using that module - the kernel is
a firmware to host a vTPM for virtual machines. Somewhat similar to SMM.
That should've been reflected somewhere in the comments and in Kconfig,
we could do better. All in all, the page fault cannot happen in that
path thus CR2 won't be trashed.
Nor this kind of code can be stepped through in a self-hosted
kernel debugger like kgdb. There are other examples of such code iiuc:
As Sean mentioned, you do have to make sure that this is annotated as
noinstr (not instrumentable). And also just use assembly - KVM started with
a similar asm block, though without the sketchy "register asm",
Ooh, yeah, don't use "register asm". I missed that when I peeked at the code.
Using "register asm" will most definitely cause problems, because the compiler
doesn't track usage in C code, i.e. will happily use the GPR and clobber your
asm value in the process. That inevitably leads to very confusing and somewhat
transient errors. E.g. if someone inserts a printk for debugging, the call to
printk can clobber the very state it's trying to print.
and I was initially skeptical but using a dedicated .S file was absolutely
the right thing to do.
+1000 to putting the assembly in a .S file. I too was a bit skeptical about
moving the entire sequence into proper assembly; thankfully, some non-KVM folks
talked us into it :-)
Thank you so much Sean and Paolo for your valuable inputs. I will try
out these things. Summarizing the suggestions here:
* Use noinstr (no instrumentation)
* Have separate .S file
* Don't use "register asm".
* Use static calls for solving IBT problems
* RAX:RCX is probably ok to be used, considering ABI. Whether we would
still need to use STACK_FRAME_NON_STANDARD, I am not sure, but I will
see based on how it goes.
I hope this addresses the concerns Peter raised. If there's anything I
might have missed, I'm happy to make further adjustments if needed.
Regards,
Naman
