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
