Re: [kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Sean Christopherson <seanjc@google.com>]

On Wed, Jun 14, 2023, Mathias Krause wrote:
> On 13.06.23 02:32, Sean Christopherson wrote:
> > On Thu, Apr 13, 2023, Mathias Krause wrote:
> >> Use an array type to refer to the code label 'ret_to_kernel'.
> > 
> > Why?  Taking the address of a label when setting what is effectively the target
> > of a branch seems more intuitive than pointing at an array (that's not an array).
> 
> Well, the flexible array notation is what my understanding of referring
> to a "label" defined in ASM is. I'm probably biased, as that's a pattern
> used a lot in the Linux kernel but trying to look at the individual
> semantics may make it clearer why I prefer 'extern char sym[]' over
> 'extern char sym'.
> 
> The current code refers to the code sequence starting at 'ret_to_kernel'
> by creating an alias of it's first instruction byte. However, we're not
> interested in the first instruction byte at all. We want a symbolic
> handle of the begin of that sequence, which might be an unknown number
> of bytes. But that's also a detail that doesn't matter. We only what to
> know the start. By referring to it as 'extern char' implies that there
> is at least a single byte. (Let's ignore the hair splitting about just
> taking the address vs. actually dereferencing it (which we do not).) By
> looking at another code example, that byte may not actually be there!
> >From x86/vmx_tests.c:
> 
>   extern char vmx_mtf_pdpte_guest_begin;
>   extern char vmx_mtf_pdpte_guest_end;
> 
>   asm("vmx_mtf_pdpte_guest_begin:\n\t"
>       "mov %cr0, %rax\n\t"    /* save CR0 with PG=1                 */
>       "vmcall\n\t"            /* on return from this CR0.PG=0       */
>       "mov %rax, %cr0\n\t"    /* restore CR0.PG=1 to enter PAE mode */
>       "vmcall\n\t"
>       "retq\n\t"
>       "vmx_mtf_pdpte_guest_end:");
> 
> The byte referred to via &vmx_mtf_pdpte_guest_end may not even be mapped
> due to -ftoplevel-reorder possibly putting that asm block at the very
> end of the compilation unit.
> 
> By using 'extern char []' instead this nastiness is avoided by referring
> to an unknown sized byte sequence starting at that symbol (with zero
> being a totally valid length). We don't need to know how many bytes
> follow the label. All we want to know is its address. And that's what an
> array type provides easily.

I think my hangup is that arrays make me think of data, not code.  I wouldn't
object if this were new code, but I dislike changing existing code to something
that's arguably just as flawed.

What if we declare the label as a function?  That's not exactly correct either,
but IMO it's closer to the truth, and let's us document that the kernel trampoline
has a return value, i.e. preserves/outputs RAX.

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index c3ec0ad7..a46a369a 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -31,17 +31,18 @@ static void restore_exec_to_jmpbuf_exception_handler(struct ex_regs *regs)
 #endif
 }
 
+uint64_t ret_to_kernel(void);
+
 uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
                uint64_t arg1, uint64_t arg2, uint64_t arg3,
                uint64_t arg4, bool *raised_vector)
 {
-       extern char ret_to_kernel;
        volatile uint64_t rax = 0;
        static unsigned char user_stack[USERMODE_STACK_SIZE];
        handler old_ex;
 
        *raised_vector = 0;
-       set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
+       set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
        old_ex = handle_exception(fault_vector,
                                  restore_exec_to_jmpbuf_exception_handler);