Re: [RFC v7 08/21] um: add nommu mode for UML library mode

[Hajime Tazaki <thehajime@gmail.com>]

Hello Johannes,

On Thu, 08 Oct 2020 00:44:12 +0900,
Johannes Berg wrote:
> 
> On Tue, 2020-10-06 at 18:44 +0900, Hajime Tazaki wrote:
> > This patch introduces the nommu operation with UML code so that host
> > interface can be shrineked for broader environments support.
> 
> shrunk
> 
> > The nommu mode is implemneted as SUBARCH of arch/um, which places at
> 
> implemented

thanks; will fix both in the next round.

> > arch/um/nommu. This SUBARCH defines mode-specific code of memory
> > management as well as thread implementation, along with the uapi headers
> > to be exported to users.
> > 
> > The headers we introduce in this patch are simple wrappers to the
> > asm-generic headers or stubs for things we don't support, such as
> > ptrace, DMA, signals, ELF handling and low level processor operations.
> > 
> > nommu mode shares most of arch/um code (irq, thread_info, process
> > scheduling) but implements its own facilities, which are memory
> > management (nommu), thread primitives (struct arch_thread), system call
> > interface, and console.
> > 
> > The outlook of updated directory structure is as follows:
> > 
> >    arch/um
> >    |-- configs         (untouched)
> >    |-- drivers         unuse stdio_console.c for !MMU
> >    |-- include
> >    |   |-- asm         updated with !CONFIG_MMU
> >    |   |-- linux       (untouched)
> >    |   `-- shared      updated with new functions
> >    |       `-- skas    (untouched)
> >    |   `-- uapi        added for upai header installation
> >    |-- kernel          updated to integrate with !MMU
> >    |   `-- skas        (untouched, don't use for !MMU)
> >    |-- nommu           SUBARCH dir (internally =um/nommu)
> >    |   |-- include
> >    |   |   |-- asm     headers for subarch specific info
> >    |   |   `-- uapi    headers for user-visible APIs
> >    |   `-- um
> >    |       `-- shared  headers for subarch specific info
> >    `-- scripts         added a script for header installation
> 
> That seems awkward. Might be nice now for "as little changes as
> possible", but eventually it seems it would be better to separate that
> out into arch/um/{common,nommu,mmu}/ or something like that?
> 
> Or perhaps something like
> 
> 	arch/um/{common,lkl,full}
> 
> or something? Not sure I like 'full' though. 'vm'? Hmm.

(I will leave this to the other thread if any)

> (also, scripts/ doesn't exist in this patch?)
> 
> > +++ b/arch/um/nommu/Makefile
> > @@ -0,0 +1 @@
> > +#
> 
> hmm? maybe add a comment why an empty makefile is needed.

Thanks, we will.

> > diff --git a/arch/um/nommu/Makefile.um b/arch/um/nommu/Makefile.um
> > new file mode 100644
> > index 000000000000..3808462d8283
> > --- /dev/null
> > +++ b/arch/um/nommu/Makefile.um
> 
> [...]
> 
> > +ifeq ($(shell uname -s), Linux)
> > +NPROC=$(shell nproc)
> > +else # e.g., FreeBSD
> > +NPROC=$(shell sysctl -n hw.ncpu)
> > +endif
> 
> That seems very inappropriate here.

Will cut the FreeBSD part.

> > +
> > +um_headers_install: $(objtree)/$(HOST_DIR)/include/generated/uapi/asm/syscall_defs.h headers
> > +# if syscall_defs.h is newer than generated headers (autoconf.h)
> > +	$(Q)if [ ! -r $(objtree)/tools/um/include/lkl/autoconf.h ] || \
> > +	  [ $< -nt $(objtree)/tools/um/include/lkl/autoconf.h ]; then \
> > +		$(srctree)/$(ARCH_DIR)/scripts/headers_install.py \
> > +			$(subst -j,-j$(NPROC),$(findstring -j,$(MAKEFLAGS))) \
> 
> Yeah ... just don't.

Will clean up this part.

> > +++ b/arch/um/nommu/include/asm/atomic.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __UM_NOMMU_ATOMIC_H
> > +#define __UM_NOMMU_ATOMIC_H
> > +
> > +#include <asm-generic/atomic.h>
> > +
> > +#ifndef CONFIG_GENERIC_ATOMIC64
> > +#include "atomic64.h"
> > +#endif /* !CONFIG_GENERIC_ATOMIC64 */
> > +
> > +#endif
> > diff --git a/arch/um/nommu/include/asm/atomic64.h b/arch/um/nommu/include/asm/atomic64.h
> > new file mode 100644
> > index 000000000000..949360dea7af
> > --- /dev/null
> > +++ b/arch/um/nommu/include/asm/atomic64.h
> 
> That doesn't make sense to me, you can control CONFIG_GENERIC_ATOMIC64
> to be on, and don't need the ifdef and this file?

We were suggested to not use GENERIC_ATOMIC64 on 64bit build during
our v3 patchset.  So we actually control CONFIG_GENERIC_ATOMIC64 to be
on only when !64BIT, and thus need atomic64.h for the 64BIT build.

https://lwn.net/ml/linux-arch/20200205093454.GG14879@hirez.programming.kicks-ass.net/

> > diff --git a/arch/um/nommu/include/asm/bitsperlong.h b/arch/um/nommu/include/asm/bitsperlong.h
> > new file mode 100644
> > index 000000000000..a150cee41e75
> > --- /dev/null
> > +++ b/arch/um/nommu/include/asm/bitsperlong.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __UM_NOMMU_BITSPERLONG_H
> > +#define __UM_NOMMU_BITSPERLONG_H
> > +
> > +#include <uapi/asm/bitsperlong.h>
> > +
> > +#define BITS_PER_LONG __BITS_PER_LONG
> > +
> > +#define BITS_PER_LONG_LONG 64
> > +
> > +#endif
> 
> That's very similar to the contents of include/asm-
> generic/bitsperlong.h, add comments why it's needed?

thanks, it seems that this is unnecessary now.  will fix them.

> > diff --git a/arch/um/nommu/include/asm/byteorder.h b/arch/um/nommu/include/asm/byteorder.h
> > new file mode 100644
> > index 000000000000..920a5fd26cad
> > --- /dev/null
> > +++ b/arch/um/nommu/include/asm/byteorder.h
> > @@ -0,0 +1,7 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __UM_NOMMU_BYTEORDER_H
> > +#define __UM_NOMMU_BYTEORDER_H
> > +
> > +#include <uapi/asm/byteorder.h>
> > +
> > +#endif
> 
> Not sure why this file even exists, it doesn't for any other arch
> (except for h8300 which seems odd).

You're right.  We will remove this.

> > diff --git a/arch/um/nommu/include/asm/elf.h b/arch/um/nommu/include/asm/elf.h
> > new file mode 100644
> > index 000000000000..edcf63edeed1
> > --- /dev/null
> > +++ b/arch/um/nommu/include/asm/elf.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __UM_NOMMU_ELF_H
> > +#define __UM_NOMMU_ELF_H
> > +
> > +#define elf_check_arch(x) 0
> 
> Please add a comment ...
>
> > +#ifdef CONFIG_64BIT
> > +#define ELF_CLASS ELFCLASS64
> > +#else
> > +#define ELF_CLASS ELFCLASS32
> > +#endif
> > +
> > +#define elf_gregset_t long
> > +#define elf_fpregset_t double
> 
> Also here.

thanks, will add the comments.

> > +#ifndef __UM_NOMMU_SCHED_H
> > +#define __UM_NOMMU_SCHED_H
> > +
> > +#include <linux/sched.h>
> > +#include <uapi/asm/host_ops.h>
> > +
> > +static inline void thread_sched_jb(void)
> > +{
> > +	if (test_ti_thread_flag(current_thread_info(), TIF_HOST_THREAD)) {
> > +		set_ti_thread_flag(current_thread_info(), TIF_SCHED_JB);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		lkl_ops->jmp_buf_set(&current_thread_info()->task->thread.arch.sched_jb,
> > +				     schedule);
> > +	} else {
> > +		lkl_bug("%s can be used only for host task", __func__);
> > +	}
> > +}
> 
> What's "thread_sched_jb"? Doesn't really seem to exist? Neither does
> "TIF_SCHED_JB", at least in my kernel?
> 
> Looks like that's added by a later patch, should probably then add this
> file also only later so it's actually consistent and one can review all
> the pieces together.

I understand. I will move this file to the later patch (11/21).

> > +++ b/arch/um/nommu/include/uapi/asm/Kbuild
> > @@ -0,0 +1,6 @@
> > +# UAPI Header export list
> > +
> > +generated-y += syscall_defs.h
> > +
> > +# no generated-y since we need special user headers handling
> > +# see arch/um/script/headers_install.py
> 
> Also doesn't exist yet.

The message is incorrect since uapi header infra has changed.
This also applies to the comment in arch/um/nommu/include/asm/Kbuild.

Will fix both in the next round.

> > +++ b/arch/um/nommu/um/delay.c
> 
> why the double um/ path?

This is because now in arch/um/Makefile,

 - SUBARCH := um/nommu
 - HEADER_ARCH := $(SUBARCH)
 - HOST_DIR := arch/$(HEADER_ARCH)
 - core-y += $(HOST_DIR)/um

the last line expands as arch/um/nommu/um.

> > +/* skas/process.c */
> > +void halt_skas(void)
> > +{}
> 
> nit: missing blank line after this

will fix this and other parts of the whole patchset.

> > +int is_skas_winch(int pid, int fd, void *data)
> > +{
> > +	return 0;
> > +}
> > +void reboot_skas(void)
> > +{}
> > +
> > +int __init start_uml(void)
> > +{
> > +	return 0;
> > +}
> 
> That seems odd?
> 
> Might be better to have an own os.h, or split that into os-uml.h and
> os-lkl.h or so?

Okay, we will think about this.

-- Hajime