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);
