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

[Hajime Tazaki<thehajime@gmail.com>]

On Fri, 25 Oct 2024 18:11:01 +0900,
Johannes Berg wrote:
> 
> (I should say, I'm still reading through this, and haven't formed an
> overall opinion. Just nitpicking on the details as I see them for now)

thanks anyway.  looking forward to any opinions.

> > +#endif
> > +
> >  
> >  #include <asm-generic/mmu_context.h>
> 
> extra newline

will fix it.

> >  /* tlb.c */
> > +#ifdef CONFIG_MMU
> >  extern void report_enomem(void);
> > +#else
> > +static inline void report_enomem(void)
> > +{
> > +}
> > +#endif
> 
> Should that really do _nothing_? Perhaps it's not called at all in no-
> MMU, but then you don't need it, but otherwise it seems it should do
> something even if it's just panic()?

it is called also in !MMU.  I'll think to figure out how the function
is shared.
> 
> >  	brk_end = (unsigned long) UML_ROUND_UP(sbrk(0));
> > +#ifdef CONFIG_MMU
> >  	map_memory(brk_end, __pa(brk_end), uml_reserved - brk_end, 1, 1, 0);
> > +#else
> > +	map_memory(brk_end, __pa(brk_end), uml_reserved - brk_end, 1, 1, 1);
> > +#endif
> 
> That seems much simpler as
> 
> 	map_memory(.....,
> 		   !IS_ENABLED(CONFIG_MMU));

looks nice, will fix it.

> > +#ifdef UML_CONFIG_MMU
> >  	loc = mmap64((void *) virt, len, prot, MAP_SHARED | MAP_FIXED,
> >  		     fd, off);
> > +#else
> > +	loc = mmap64((void *) virt, len, prot, MAP_SHARED | MAP_FIXED | MAP_ANONYMOUS,
> > +		     fd, off);
> > +#endif
> 
> Same here,
> 
> 	mmap64(....
> 	       MAP_SHARED | MAP_FIXED |
> 		IS_ENABLED(CONFIG_MMU) ? MAP_ANONYMOUS : 0,
> 	       ...);

since this is part under os-Linux and we cannot use kconfig.h (IIUC)
feature (e.g., IS_ENABLED). but I'll reformat it to simplify instead
of duplicating same lines.

-- Hajime