Hi Geert,
On Mon, Feb 27, 2023 at 01:31:23PM +0100, Geert Uytterhoeven wrote:
> Hi Mike,
>
> On Mon, Feb 27, 2023 at 12:34 PM Mike Rapoport <r...@kernel.org> wrote:
> > On Mon, Feb 27, 2023 at 10:42:34PM +1300, Michael Schmitz wrote:
> > > Am 27.02.2023 um 21:26 schrieb Geert Uytterhoeven:
> > > > > On Mon, 27 Feb 2023, Michael Schmitz wrote:
> > > > > > I wonder whether Finn's memtest patch merely exposed another MM bug
> > > > >
> > > > > A kernel patch may be easier than a bootloader patch (even if this is
> > > > > a
> > > > > bootloader bug) particularly if it affects multiple platforms.
> > > > >
> > > > > A partial revert of my patch (below) will probably avoid the issue,
> > > > > but
> > > > > with the side effect that use of memtest will clobber the initrd.
> > > >
> > > > Which we can avoid, by moving the ramdisk handling inside paging_init().
> > > >
> > > > > The initrd and memtest features aren't usually needed together. At the
> > > > > time when I needed the memtest feature I did not have confidence in
> > > > > the
> > > > > hardeare. An initrd wasn't very useful at that point.
> > > > >
> > > > > diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
> > > > > index 3a2bb2e8fdad..92f1b9268dff 100644
> > > > > --- a/arch/m68k/kernel/setup_mm.c
> > > > > +++ b/arch/m68k/kernel/setup_mm.c
> > > > > @@ -326,6 +326,8 @@ void __init setup_arch(char **cmdline_p)
> > > > > panic("No configuration setup");
> > > > > }
> > > > >
> > > > > + paging_init();
> > > > > +
> > > > > #ifdef CONFIG_BLK_DEV_INITRD
> > > > > if (m68k_ramdisk.size) {
> > > > > memblock_reserve(m68k_ramdisk.addr,
> > > > > m68k_ramdisk.size);
> > > >
> > > > Presumably something in memblock_reserve() relies on having
> > > > called paging_init() before?
> >
> > memblock_reserve() does not rely on paging_init() as it operates on
> > physical addresses and it does not care if memory was already registered.
> >
> > What does rely on paging_init() it's phys_to_virt() in the line after
> > memblock_reserve():
> >
> > initrd_start = (unsigned
> > long)phys_to_virt(m68k_ramdisk.addr);
> > initrd_end = initrd_start + m68k_ramdisk.size;
> >
> > So to have both memtest and initrd we'd need something like
> >
> > memblock_reserve(m68k_ramdisk.addr, m68k_ramdisk.size);
> >
> > paging_init() {
> > /* setup page tables and memblock */
> > early_memtest();
> > }
> >
> > initrd_start = (unsigned long)phys_to_virt(m68k_ramdisk.addr);
> >
> > or
> >
> > paging_init(); /* without early_memtest() */
> >
> > memblock_reserve(m68k_ramdisk.addr, m68k_ramdisk.size);
> > initrd_start = (unsigned long)phys_to_virt(m68k_ramdisk.addr);
> >
> > early_memtest();
>
> Of course... /me bangs his head against the TFT for not having
> realized before the values saved into initrd_{start,end} are not just
> for printing in the pr_info() line...
Happens to the best of us :)
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
--
Sincerely yours,
Mike.
