On Tue, 2024-12-03 at 13:23 +0900, Hajime Tazaki wrote:
>
> +++ b/arch/um/kernel/process.c
> @@ -117,13 +117,17 @@ void new_thread_handler(void)
> * callback returns only if the kernel thread execs a process
> */
> fn(arg);
> +#ifndef CONFIG_MMU
> + arch_switch_to(current);
> +#endif
> userspace(¤t->thread.regs.regs);
that doesn't make sense, arch_switch_to() does nothing anyway on 64-bit
> /* Called magically, see new_thread_handler above */
> static void fork_handler(void)
> {
> - schedule_tail(current->thread.prev_sched);
> + if (current->thread.prev_sched)
> + schedule_tail(current->thread.prev_sched);
Why is that NULL on nommu?
> @@ -134,6 +138,33 @@ static void fork_handler(void)
>
> current->thread.prev_sched = NULL;
>
> +#ifndef CONFIG_MMU
again, don't sprinkle ifdefs around the C code files - make inlines in a
header file or something
> + /*
> + * child of vfork(2) comes here.
> + * clone(2) also enters here but doesn't need to advance the %rsp.
> + *
> + * This fork can only come from libc's vfork, which
> + * does this:
> + * popq %%rdx;
> + * call *%rax; // zpoline => __kernel_vsyscall
> + * pushq %%rdx;
or maybe not zpoline ... so maybe need to update this
> +++ b/arch/um/os-Linux/skas/process.c
> @@ -144,6 +144,7 @@ void wait_stub_done(int pid)
>
> extern unsigned long current_stub_stack(void);
>
> +#ifdef CONFIG_MMU
I'll stop commenting on ifdef sprinkling :)
> +++ b/arch/x86/um/asm/processor.h
> @@ -38,6 +38,18 @@ static __always_inline void cpu_relax(void)
>
> #define task_pt_regs(t) (&(t)->thread.regs)
>
> +#ifndef CONFIG_MMU
> +#define task_top_of_stack(task) \
> +({ \
> + unsigned long __ptr = (unsigned long)task->stack; \
> + __ptr += THREAD_SIZE; \
> + __ptr; \
> +})
> +
> +extern long current_top_of_stack;
> +extern long current_ptregs;
> +#endif
no need for "extern".
you only use all that once, does it need to be here?
> +
> #include <asm/processor-generic.h>
>
> #endif
> diff --git a/arch/x86/um/do_syscall_64.c b/arch/x86/um/do_syscall_64.c
> index 5d0fa83e7fdc..ca468caff729 100644
> --- a/arch/x86/um/do_syscall_64.c
> +++ b/arch/x86/um/do_syscall_64.c
> @@ -1,14 +1,43 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +//#define DEBUG 1
please remove
> #include <linux/kernel.h>
> #include <linux/ptrace.h>
> #include <kern_util.h>
> #include <sysdep/syscalls.h>
> #include <os.h>
>
> +/*
> + * save/restore the return address stored in the stack, as the child
> overwrites
> + * the contents after returning to userspace (i.e., by push %rdx).
> + *
> + * see the detail in fork_handler().
> + */
> +static void *vfork_save_stack(void)
> +{
> + unsigned char *stack_copy;
> +
> + stack_copy = kzalloc(8, GFP_KERNEL);
Using a heap allocation to track 8 bytes, when the pointer to the
allocation is already 8 bytes (you're on 64-bit) seems ... rather
wasteful?
I also don't see you ever free it? Restore probably should, but anyway,
it shouldn't exist.
johannes
