[PATCH v3 03/13] um: nommu: memory handling

[Johannes Berg]

On Tue, 2024-12-03 at 13:23 +0900, Hajime Tazaki wrote:
> 
> +++ b/arch/um/include/asm/futex.h
> @@ -8,7 +8,11 @@
>  
>  
>  int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
> +#ifdef CONFIG_MMU
>  int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>  			      u32 oldval, u32 newval);
> +#else
> +#include <asm-generic/futex.h>
> +#endif

That seems somewhat problematic since it also defines
arch_futex_atomic_op_inuser ...

> +++ b/arch/um/include/shared/os.h
> @@ -195,7 +195,13 @@ extern void get_host_cpu_features(
>  extern int create_mem_file(unsigned long long len);
>  
>  /* tlb.c */
> +#ifdef CONFIG_MMU
>  extern void report_enomem(void);
> +#else
> +static inline void report_enomem(void)
> +{
> +}
> +#endif

That still seems simply wrong? Why is that even called, and why should
it do *nothing*?

I'm thinking you might just have patch sequence issues here - I can't
really see why this would be called at all, eventually.

> @@ -78,6 +79,7 @@ void __init mem_init(void)
>   * Create a page table and place a pointer to it in a middle page
>   * directory entry.
>   */
> +#ifdef CONFIG_MMU
>  static void __init one_page_table_init(pmd_t *pmd)
>  {
>  	if (pmd_none(*pmd)) {
> @@ -149,6 +151,12 @@ static void __init fixrange_init(unsigned long start, unsigned long end,
>  		j = 0;
>  	}
>  }
> +#else
> +static void __init fixrange_init(unsigned long start, unsigned long end,
> +				 pgd_t *pgd_base)
> +{
> +}
> +#endif

Really not a fan of all these randomly placed ifdefs ...

> +#ifdef CONFIG_MMU
>  static const pgprot_t protection_map[16] = {
>  	[VM_NONE]					= PAGE_NONE,
>  	[VM_READ]					= PAGE_READONLY,
> @@ -249,3 +258,4 @@ static const pgprot_t protection_map[16] = {
>  	[VM_SHARED | VM_EXEC | VM_WRITE | VM_READ]	= PAGE_SHARED
>  };
>  DECLARE_VM_GET_PAGE_PROT
> +#endif

Same here.

I think we can do better - perhaps move some code to mmu.c and nommu.c
or something like that.

> diff --git a/arch/um/kernel/physmem.c b/arch/um/kernel/physmem.c
> index a74f17b033c4..f55d46dbe173 100644
> --- a/arch/um/kernel/physmem.c
> +++ b/arch/um/kernel/physmem.c
> @@ -84,7 +84,11 @@ void __init setup_physmem(unsigned long start, unsigned long reserve_end,
>  		exit(1);
>  	}
>  
> +#ifdef CONFIG_MMU
>  	physmem_fd = create_mem_file(len);
> +#else
> +	physmem_fd = -1;
> +#endif

same here, create_mem_file() can just be in a file only built for mmu,
and otherwise be an inline that returns -1?

> +#ifdef CONFIG_MMU
>  	/*
>  	 * Special kludge - This page will be mapped in to userspace processes
>  	 * from physmem_fd, so it needs to be written out there.
>  	 */
>  	os_seek_file(physmem_fd, __pa(__syscall_stub_start));
>  	os_write_file(physmem_fd, __syscall_stub_start, PAGE_SIZE);
> +#endif

That doesn't even do anything if the fd is -1, do we need the ifdef? ;-)

Still better as "if (IS_ENABLED())" or something anyway though.

> +++ b/arch/um/kernel/trap.c
> @@ -24,6 +24,7 @@
>  int handle_page_fault(unsigned long address, unsigned long ip,
>  		      int is_write, int is_user, int *code_out)
>  {
> +#ifdef CONFIG_MMU
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	pmd_t *pmd;
> @@ -129,6 +130,9 @@ int handle_page_fault(unsigned long address, unsigned long ip,
>  		goto out_nosemaphore;
>  	pagefault_out_of_memory();
>  	return 0;
> +#else
> +	return -EFAULT;
> +#endif
>  }

same comments here ... try not to sprinkle ifdefs everywhere.

johannes