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
