Re: [PATCH] x86/hyperv: Export hv_hypercall_pg unconditionally

[Naman Jain <namjain@linux.microsoft.com>]

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