Re: [RFC v8 06/20] um: add UML library mode

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2021-01-20 at 11:27 +0900, Hajime Tazaki wrote:
> 
> +++ b/arch/um/lkl/include/asm/atomic.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __UM_LIBMODE_ATOMIC_H
> +#define __UM_LIBMODE_ATOMIC_H
> +
> +#include <asm-generic/atomic.h>
> +
> +#ifndef CONFIG_GENERIC_ATOMIC64
> +#include "atomic64.h"
> +#endif /* !CONFIG_GENERIC_ATOMIC64 */

That's not actually configurable, is it? When is this on or off?


> +static inline void cpu_relax(void)
> +{
> +	unsigned long flags;
> +
> +	/* since this is usually called in a tight loop waiting for some
> +	 * external condition (e.g. jiffies) lets run interrupts now to allow
> +	 * the external condition to propagate
> +	 */
> +	local_irq_save(flags);
> +	local_irq_restore(flags);

Hmm?

If interrupts are enabled, then you'll get them anyway, and if not, then
this does nothing?

> +static inline void arch_copy_thread(struct arch_thread *from,
> +				    struct arch_thread *to)
> +{
> +	panic("unimplemented %s: fork isn't supported yet", __func__);
> +}

"yet" seems kind of misleading - I mean, it really *won't* be, since
that's basically what UML is, no?

I mean, in principle, nothing actually stops you from building a
linux.so instead of linux binary, and having main() renamed to
linux_main() so the application can start up whenever it needs to, or
something like that?



> +#endif
> diff --git a/arch/um/lkl/include/asm/ptrace.h b/arch/um/lkl/include/asm/ptrace.h
> new file mode 100644
> index 000000000000..83f4e10fb677
> --- /dev/null
> +++ b/arch/um/lkl/include/asm/ptrace.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __UM_LIBMODE_PTRACE_H
> +#define __UM_LIBMODE_PTRACE_H
> +
> +#include <linux/errno.h>
> +
> +static int reg_dummy __attribute__((unused));
> +
> +#define PT_REGS_ORIG_SYSCALL(r) (reg_dummy)
> +#define PT_REGS_SYSCALL_RET(r) (reg_dummy)
> +#define PT_REGS_SET_SYSCALL_RETURN(r, res) (reg_dummy = (res))
> +#define REGS_SP(r) (reg_dummy)
> +
> +#define user_mode(regs) 0
> +#define kernel_mode(regs) 1
> +#define profile_pc(regs) 0
> +#define user_stack_pointer(regs) 0
> +
> +extern void new_thread_handler(void);
> +
> +#endif
> diff --git a/arch/um/lkl/include/asm/segment.h b/arch/um/lkl/include/asm/segment.h
> new file mode 100644
> index 000000000000..1f6746866b8b
> --- /dev/null
> +++ b/arch/um/lkl/include/asm/segment.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __UM_LIBMODE_SEGMENT_H
> +#define __UM_LIBMODE_SEGMENT_H
> +
> +typedef struct {
> +	unsigned long seg;
> +} mm_segment_t;
> +
> +#endif /* _ASM_LKL_SEGMENT_H */
> diff --git a/arch/um/lkl/include/uapi/asm/bitsperlong.h b/arch/um/lkl/include/uapi/asm/bitsperlong.h
> new file mode 100644
> index 000000000000..f6657ba0ff9b
> --- /dev/null
> +++ b/arch/um/lkl/include/uapi/asm/bitsperlong.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __UM_LIBMODE_UAPI_BITSPERLONG_H
> +#define __UM_LIBMODE_UAPI_BITSPERLONG_H
> +
> +#ifdef CONFIG_64BIT
> +#define __BITS_PER_LONG 64
> +#else
> +#define __BITS_PER_LONG 32
> +#endif
> +
> +#include <asm-generic/bitsperlong.h>

First, that include does nothing. Second, the comment there says:

/*
 * There seems to be no way of detecting this automatically from user
 * space, so 64 bit architectures should override this in their
 * bitsperlong.h. In particular, an architecture that supports
 * both 32 and 64 bit user space must not rely on CONFIG_64BIT
 * to decide it, but rather check a compiler provided macro.
 */


So you're doing exactly what it says *not* to?

In fact, that totally makes sense - I mean, this is *uapi* and
applications don't have access to CONFIG_ defines etc.

Do you expose that somehow to LKL users that makes this OK-ish? But it's
still very confusing, and you've not made sure the necessary header is
actually included. IIUC, lkl builds a (shared?) library, so it won't
really work this way?

> +++ b/arch/um/lkl/include/uapi/asm/byteorder.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __UM_LIBMODE_UAPI_BYTEORDER_H
> +#define __UM_LIBMODE_UAPI_BYTEORDER_H
> +
> +#if defined(CONFIG_BIG_ENDIAN)
> +#include <linux/byteorder/big_endian.h>

Same here.

> +++ b/arch/um/lkl/um/delay.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/jiffies.h>
> +#include <linux/delay.h>
> +#include <os.h>
> +
> +void __ndelay(unsigned long nsecs)
> +{
> +	long long start = os_nsecs();
> +
> +	while (os_nsecs() < start + nsecs)
> +		;

At least do something like cpu_relax()? Although ... you made that do
nothing, so you probably want an arch-specific thing here?

johannes