Re: [PATCH v2] x86/hyperv: Use __naked attribute to fix stackless C function

["Ard Biesheuvel" <ardb@kernel.org>]

On Sat, 28 Feb 2026, at 11:17, Uros Bizjak wrote:
> On Fri, Feb 27, 2026 at 11:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> hv_crash_c_entry() is a C function that is entered without a stack,
>> and this is only allowed for functions that have the __naked attribute,
>> which informs the compiler that it must not emit the usual prologue and
>> epilogue or emit any other kind of instrumentation that relies on a
>> stack frame.
>>
>> So split up the function, and set the __naked attribute on the initial
>> part that sets up the stack, GDT, IDT and other pieces that are needed
>> for ordinary C execution. Given that function calls are not permitted
>> either, use the existing long return coded in an asm() block to call the
>> second part of the function, which is an ordinary function that is
>> permitted to call other functions as usual.
>>
>> Cc: Mukesh Rathor <mrathor@linux.microsoft.com>
>> Cc: Wei Liu <wei.liu@kernel.org>
>> Cc: Uros Bizjak <ubizjak@gmail.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: linux-hyperv@vger.kernel.org
>> Fixes: 94212d34618c ("x86/hyperv: Implement hypervisor RAM collection into vmcore")
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>> v2: apply some asm tweaks suggested by Uros and Andrew
>>
>>  arch/x86/hyperv/hv_crash.c | 79 ++++++++++----------
>>  1 file changed, 41 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
>> index 92da1b4f2e73..1c0965eb346e 100644
>> --- a/arch/x86/hyperv/hv_crash.c
>> +++ b/arch/x86/hyperv/hv_crash.c
>> @@ -107,14 +107,12 @@ static void __noreturn hv_panic_timeout_reboot(void)
>>                 cpu_relax();
>>  }
>>
>> -/* This cannot be inlined as it needs stack */
>> -static noinline __noclone void hv_crash_restore_tss(void)
>> +static void hv_crash_restore_tss(void)
>>  {
>>         load_TR_desc();
>>  }
>>
>> -/* This cannot be inlined as it needs stack */
>> -static noinline void hv_crash_clear_kernpt(void)
>> +static void hv_crash_clear_kernpt(void)
>>  {
>>         pgd_t *pgd;
>>         p4d_t *p4d;
>> @@ -125,6 +123,25 @@ static noinline void hv_crash_clear_kernpt(void)
>>         native_p4d_clear(p4d);
>>  }
>>
>> +
>> +static void __noreturn hv_crash_handle(void)
>> +{
>> +       hv_crash_restore_tss();
>> +       hv_crash_clear_kernpt();
>> +
>> +       /* we are now fully in devirtualized normal kernel mode */
>> +       __crash_kexec(NULL);
>> +
>> +       hv_panic_timeout_reboot();
>> +}
>> +
>> +/*
>> + * __naked functions do not permit function calls, not even to __always_inline
>> + * functions that only contain asm() blocks themselves. So use a macro instead.
>> + */
>> +#define hv_wrmsr(msr, val) \
>> +       asm("wrmsr" :: "c"(msr), "a"((u32)val), "d"((u32)(val >> 32)) : "memory")
>
> This one should be defined as "asm volatile", otherwise the compiler
> will remove it (it has no outputs used later in the code!).

An asm() block with only input operands does not need to be marked as volatile to prevent it from being optimized away.

> Also, it
> should be defined as "asm volatile" when it is important that the insn
> stays where it is, relative to other "asm volatile"s. Otherwise, the
> compiler is free to schedule other insns, including other "asm
> volatile"s around . Since this macro is also used to update
> MSR_GS_BASE (so it affects memory in case of %gs prefixed access),
> "memory" clobber should remain).
>

All the other asm() blocks except the last one read from memory, and hv_msr() has a memory clobber. So I don't think there is any legal transformation that the compiler might apply except perhaps re-ordering it with the final asm() block doing the long return.

So I don't see any reason for volatile on hv_msr(). However, I do see a potential issue with the compiler assuming that code after the final asm() block is reachable, and calling unreachable() is not permitted [by Clang] due to the __naked attribute.

Would it be better to add a memory clobber to that one as well?