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

[Uros Bizjak]

On Thu, Feb 26, 2026 at 11:48 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> Hi Uros,
>
> On Thu, 26 Feb 2026, at 11:35, Uros Bizjak wrote:
> > On Thu, Feb 26, 2026 at 10:51 AM Ard Biesheuvel <ardb+git@xxxxxxxxxx> wrote:
> >>
> >> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >>
> >> 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.
> >>
> >> Fixes: 94212d34618c ("x86/hyperv: Implement hypervisor RAM collection into vmcore")
> >> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >> ---
> >> Build tested only.
> >>
> >> Cc: Mukesh Rathor <mrathor@xxxxxxxxxxxxxxxxxxx>
> >> Cc: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx>
> >> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> >> Cc: Wei Liu <wei.liu@xxxxxxxxxx>
> >> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx>
> >> Cc: Long Li <longli@xxxxxxxxxxxxx>
> >> Cc: Thomas Gleixner <tglx@xxxxxxxxxx>
> >> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> >> Cc: Borislav Petkov <bp@xxxxxxxxx>
> >> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> >> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> >> Cc: Uros Bizjak <ubizjak@xxxxxxxxx>
> >> Cc: linux-hyperv@xxxxxxxxxxxxxxx
> >>
> >>  arch/x86/hyperv/hv_crash.c | 80 ++++++++++----------
> >>  1 file changed, 42 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
> >> index a78e4fed5720..d77766e8d37e 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 is the C entry point from the asm glue code after the disable hypercall.
> >>   * We enter here in IA32-e long mode, ie, full 64bit mode running on kernel
> >> @@ -133,49 +150,36 @@ static noinline void hv_crash_clear_kernpt(void)
> >>   * available. We restore kernel GDT, and rest of the context, and continue
> >>   * to kexec.
> >>   */
> >> -static asmlinkage void __noreturn hv_crash_c_entry(void)
> >> +static void __naked hv_crash_c_entry(void)
> >>  {
> >> -       struct hv_crash_ctxt *ctxt = &hv_crash_ctxt;
> >> -
> >>         /* first thing, restore kernel gdt */
> >> -       native_load_gdt(&ctxt->gdtr);
> >> +       asm volatile("lgdt %0" : : "m" (hv_crash_ctxt.gdtr));
> >>
> >> -       asm volatile("movw %%ax, %%ss" : : "a"(ctxt->ss));
> >> -       asm volatile("movq %0, %%rsp" : : "m"(ctxt->rsp));
> >> +       asm volatile("movw %%ax, %%ss" : : "a"(hv_crash_ctxt.ss));
> >> +       asm volatile("movq %0, %%rsp" : : "m"(hv_crash_ctxt.rsp));
> >>
> >> -       asm volatile("movw %%ax, %%ds" : : "a"(ctxt->ds));
> >> -       asm volatile("movw %%ax, %%es" : : "a"(ctxt->es));
> >> -       asm volatile("movw %%ax, %%fs" : : "a"(ctxt->fs));
> >> -       asm volatile("movw %%ax, %%gs" : : "a"(ctxt->gs));
> >> +       asm volatile("movw %%ax, %%ds" : : "a"(hv_crash_ctxt.ds));
> >> +       asm volatile("movw %%ax, %%es" : : "a"(hv_crash_ctxt.es));
> >> +       asm volatile("movw %%ax, %%fs" : : "a"(hv_crash_ctxt.fs));
> >> +       asm volatile("movw %%ax, %%gs" : : "a"(hv_crash_ctxt.gs));
> >>
> >> -       native_wrmsrq(MSR_IA32_CR_PAT, ctxt->pat);
> >> -       asm volatile("movq %0, %%cr0" : : "r"(ctxt->cr0));
> >> +       hv_wrmsr(MSR_IA32_CR_PAT, hv_crash_ctxt.pat);
> >> +       asm volatile("movq %0, %%cr0" : : "r"(hv_crash_ctxt.cr0));
> >>
> >> -       asm volatile("movq %0, %%cr8" : : "r"(ctxt->cr8));
> >> -       asm volatile("movq %0, %%cr4" : : "r"(ctxt->cr4));
> >> -       asm volatile("movq %0, %%cr2" : : "r"(ctxt->cr4));
> >> +       asm volatile("movq %0, %%cr8" : : "r"(hv_crash_ctxt.cr8));
> >> +       asm volatile("movq %0, %%cr4" : : "r"(hv_crash_ctxt.cr4));
> >> +       asm volatile("movq %0, %%cr2" : : "r"(hv_crash_ctxt.cr4));
> >>
> >> -       native_load_idt(&ctxt->idtr);
> >> -       native_wrmsrq(MSR_GS_BASE, ctxt->gsbase);
> >> -       native_wrmsrq(MSR_EFER, ctxt->efer);
> >> +       asm volatile("lidt %0" : : "m" (hv_crash_ctxt.idtr));
> >> +       hv_wrmsr(MSR_GS_BASE, hv_crash_ctxt.gsbase);
> >> +       hv_wrmsr(MSR_EFER, hv_crash_ctxt.efer);
> >>
> >>         /* restore the original kernel CS now via far return */
> >> -       asm volatile("movzwq %0, %%rax\n\t"
> >> -                    "pushq %%rax\n\t"
> >> -                    "pushq $1f\n\t"
> >> -                    "lretq\n\t"
> >> -                    "1:nop\n\t" : : "m"(ctxt->cs) : "rax");
> >> -
> >> -       /* We are in asmlinkage without stack frame, hence make C function
> >> -        * calls which will buy stack frames.
> >> -        */
> >> -       hv_crash_restore_tss();
> >> -       hv_crash_clear_kernpt();
> >> -
> >> -       /* we are now fully in devirtualized normal kernel mode */
> >> -       __crash_kexec(NULL);
> >> -
> >> -       hv_panic_timeout_reboot();
> >> +       asm volatile("pushq     %q0             \n\t"
> >> +                    "leaq      %c1(%%rip), %q0 \n\t"
> >
> > You can use %a1 instead of %c1(%%rip).
> >
>
> Nice.
>
> >> +                    "pushq     %q0             \n\t"
> >> +                    "lretq                     \n\t"
> >
> > No need for terminating \n\t after the last insn in the asm template.
> >
> >> +                    :: "a"(hv_crash_ctxt.cs), "i"(hv_crash_handle));
> >
> > Pedantically, you need ': "+a"(...) : "i"(...)' here.
> >
>
> Right, so the compiler knows that the register will be updated by the asm() block. But what is preventing it from writing back this value to hv_crash_ctxt.cs? The generated code doesn't seem to do so, but the semantics of "+r" suggest otherwise AIUI.
>
> The code following the asm() block is unreachable anyway, so it doesn't really matter either way in practice. Just curious ...

Oh, you just need a temporary here... the original is OK. Indeed, "+r"
will write back the value to the memory location, and this is not what
we want here.

Uros.