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

[Hajime Tazaki <thehajime@gmail.com>]

On Thu, 05 Dec 2024 01:34:49 +0900,
Johannes Berg wrote:
> 
> 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 ...

indeed, will fix it.

> > +++ 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.

report_enomem was used in trap.c, which both MMU and !MMU use.  Now I
decouple the file to contain !mmu specific code so, the above chunk
can be reverted.

> > @@ -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 ...

I decided to introduce nommu directory to avoid those 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.

will fix it.

> > 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?

ditto.

> > +#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.

I'll revert this part.

> > +++ 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.

will revert it.

-- Hajime