[PATCH] riscv: Drop setup_initrd

[PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

setup_initrd() does not appear to serve a practical purpose other than
preventing qemu boots with "-initrd" parameter, so let's drop it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/riscv/kernel/setup.c | 39 ---------------------------------------
 1 file changed, 39 deletions(-)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 2e56af3281f8..579f58a42974 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
 /* The lucky hart to first increment this variable will boot the other cores */
 atomic_t hart_lottery;
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static void __init setup_initrd(void)
-{
-	extern char __initramfs_start[];
-	extern unsigned long __initramfs_size;
-	unsigned long size;
-
-	if (__initramfs_size > 0) {
-		initrd_start = (unsigned long)(&__initramfs_start);
-		initrd_end = initrd_start + __initramfs_size;
-	}
-
-	if (initrd_start >= initrd_end) {
-		printk(KERN_INFO "initrd not found or empty");
-		goto disable;
-	}
-	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
-		printk(KERN_ERR "initrd extends beyond end of memory");
-		goto disable;
-	}
-
-	size =  initrd_end - initrd_start;
-	memblock_reserve(__pa(initrd_start), size);
-	initrd_below_start_ok = 1;
-
-	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
-		(void *)(initrd_start), size);
-	return;
-disable:
-	pr_cont(" - disabling initrd\n");
-	initrd_start = 0;
-	initrd_end = 0;
-}
-#endif /* CONFIG_BLK_DEV_INITRD */
-
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
 pgd_t trampoline_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
 
@@ -195,10 +160,6 @@ static void __init setup_bootmem(void)
 	set_max_mapnr(PFN_DOWN(mem_size));
 	max_low_pfn = pfn_base + PFN_DOWN(mem_size);
 
-#ifdef CONFIG_BLK_DEV_INITRD
-	setup_initrd();
-#endif /* CONFIG_BLK_DEV_INITRD */
-
 	early_init_fdt_reserve_self();
 	early_init_fdt_scan_reserved_mem();
 	memblock_allow_resize();
-- 
2.7.4

Re: [PATCH] riscv: Drop setup_initrd

[Christoph Hellwig]

On Thu, Aug 09, 2018 at 09:11:40PM -0700, Guenter Roeck wrote:
> setup_initrd() does not appear to serve a practical purpose other than
> preventing qemu boots with "-initrd" parameter, so let's drop it.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] riscv: Drop setup_initrd

[Palmer Dabbelt]

On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> setup_initrd() does not appear to serve a practical purpose other than
> preventing qemu boots with "-initrd" parameter, so let's drop it.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  arch/riscv/kernel/setup.c | 39 ---------------------------------------
>  1 file changed, 39 deletions(-)

I missed this for the merge window but I think it's suitable for an RC so I'll 
target it for next week.  It's at kernel.org/palmer/linux.git/fix-initrd and 
I've added it to for-next.

Thanks!

Re: [PATCH] riscv: Drop setup_initrd

[Palmer Dabbelt]

On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> setup_initrd() does not appear to serve a practical purpose other than
> preventing qemu boots with "-initrd" parameter, so let's drop it.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  arch/riscv/kernel/setup.c | 39 ---------------------------------------
>  1 file changed, 39 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 2e56af3281f8..579f58a42974 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>  /* The lucky hart to first increment this variable will boot the other cores */
>  atomic_t hart_lottery;
>
> -#ifdef CONFIG_BLK_DEV_INITRD
> -static void __init setup_initrd(void)
> -{
> -	extern char __initramfs_start[];
> -	extern unsigned long __initramfs_size;
> -	unsigned long size;
> -
> -	if (__initramfs_size > 0) {
> -		initrd_start = (unsigned long)(&__initramfs_start);
> -		initrd_end = initrd_start + __initramfs_size;
> -	}
> -
> -	if (initrd_start >= initrd_end) {
> -		printk(KERN_INFO "initrd not found or empty");
> -		goto disable;
> -	}
> -	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
> -		printk(KERN_ERR "initrd extends beyond end of memory");
> -		goto disable;
> -	}
> -
> -	size =  initrd_end - initrd_start;
> -	memblock_reserve(__pa(initrd_start), size);
> -	initrd_below_start_ok = 1;
> -
> -	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
> -		(void *)(initrd_start), size);
> -	return;
> -disable:
> -	pr_cont(" - disabling initrd\n");
> -	initrd_start = 0;
> -	initrd_end = 0;
> -}
> -#endif /* CONFIG_BLK_DEV_INITRD */
> -
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
>  pgd_t trampoline_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>
> @@ -195,10 +160,6 @@ static void __init setup_bootmem(void)
>  	set_max_mapnr(PFN_DOWN(mem_size));
>  	max_low_pfn = pfn_base + PFN_DOWN(mem_size);
>
> -#ifdef CONFIG_BLK_DEV_INITRD
> -	setup_initrd();
> -#endif /* CONFIG_BLK_DEV_INITRD */
> -
>  	early_init_fdt_reserve_self();
>  	early_init_fdt_scan_reserved_mem();
>  	memblock_allow_resize();

With this patch I can't boot in QEMU.  It might be something with my setup, 
though.  I'm testing with a unified BBL+vmlinux, and using QEMU 3.0.0-rc3 (I 
should probably update that one, but I don't remember anything going by).  Do 
you have time to take a look?  Here's my QEMU commandline

./riscv64-softmmu/qemu-system-riscv64    -nographic    -machine virt    -smp 4    -m 2G    -kernel /home/palmer/work/riscv/riscv-pk/build/bbl -object rng-random,filename=/dev/urandom,id=rng0    -device virtio-rng-device,rng=rng0    -append "console=ttyS0 ro root=/dev/vda"    -device virtio-blk-device,drive=hd0    -drive file=stage4-disk.img,format=raw,id=hd0    -device virtio-net-device,netdev=usernet    -netdev user,id=usernet,hostfwd=tcp::10000-:22

I just tagged the PR I plan to submit for RC2 tomorrow, so at least it should 
be easy for everyone to get on the same page.

Re: [PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> >setup_initrd() does not appear to serve a practical purpose other than
> >preventing qemu boots with "-initrd" parameter, so let's drop it.
> >
> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >---
> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
> > 1 file changed, 39 deletions(-)
> >
> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >index 2e56af3281f8..579f58a42974 100644
> >--- a/arch/riscv/kernel/setup.c
> >+++ b/arch/riscv/kernel/setup.c
> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
> > /* The lucky hart to first increment this variable will boot the other cores */
> > atomic_t hart_lottery;
> >
> >-#ifdef CONFIG_BLK_DEV_INITRD
> >-static void __init setup_initrd(void)
> >-{
> >-	extern char __initramfs_start[];
> >-	extern unsigned long __initramfs_size;
> >-	unsigned long size;
> >-
> >-	if (__initramfs_size > 0) {
> >-		initrd_start = (unsigned long)(&__initramfs_start);
> >-		initrd_end = initrd_start + __initramfs_size;
> >-	}
> >-
> >-	if (initrd_start >= initrd_end) {
> >-		printk(KERN_INFO "initrd not found or empty");
> >-		goto disable;
> >-	}
> >-	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
> >-		printk(KERN_ERR "initrd extends beyond end of memory");
> >-		goto disable;
> >-	}
> >-
> >-	size =  initrd_end - initrd_start;
> >-	memblock_reserve(__pa(initrd_start), size);
> >-	initrd_below_start_ok = 1;
> >-
> >-	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
> >-		(void *)(initrd_start), size);
> >-	return;
> >-disable:
> >-	pr_cont(" - disabling initrd\n");
> >-	initrd_start = 0;
> >-	initrd_end = 0;
> >-}
> >-#endif /* CONFIG_BLK_DEV_INITRD */
> >-
> > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> >
> >@@ -195,10 +160,6 @@ static void __init setup_bootmem(void)
> > 	set_max_mapnr(PFN_DOWN(mem_size));
> > 	max_low_pfn = pfn_base + PFN_DOWN(mem_size);
> >
> >-#ifdef CONFIG_BLK_DEV_INITRD
> >-	setup_initrd();
> >-#endif /* CONFIG_BLK_DEV_INITRD */
> >-
> > 	early_init_fdt_reserve_self();
> > 	early_init_fdt_scan_reserved_mem();
> > 	memblock_allow_resize();
> 
> With this patch I can't boot in QEMU.  It might be something with my setup,
> though.  I'm testing with a unified BBL+vmlinux, and using QEMU 3.0.0-rc3 (I
> should probably update that one, but I don't remember anything going by).
> Do you have time to take a look?  Here's my QEMU commandline
> 

Odd. Maybe 'RISC-V: Support separate firmware and kernel payload' creates
incompatibilities, or you have an embedded initrd in your kernel image
and that somehow doesn't work (I thought I tested that but maybe I didn't).

Unfortunately, I have no means to generate a unified bbl+vmlinux,
I would have to set it up, and I won't have time anytime soon to do that.
Guess we'll have to live with broken initrd for non-unified builds.
I'll just drop the initrd boot tests from my setup.

Guenter

> ./riscv64-softmmu/qemu-system-riscv64    -nographic    -machine virt    -smp 4    -m 2G    -kernel /home/palmer/work/riscv/riscv-pk/build/bbl -object rng-random,filename=/dev/urandom,id=rng0    -device virtio-rng-device,rng=rng0    -append "console=ttyS0 ro root=/dev/vda"    -device virtio-blk-device,drive=hd0    -drive file=stage4-disk.img,format=raw,id=hd0    -device virtio-net-device,netdev=usernet    -netdev user,id=usernet,hostfwd=tcp::10000-:22
> 
> I just tagged the PR I plan to submit for RC2 tomorrow, so at least it
> should be easy for everyone to get on the same page.

Re: [PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> >setup_initrd() does not appear to serve a practical purpose other than
> >preventing qemu boots with "-initrd" parameter, so let's drop it.
> >
> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >---
> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
> > 1 file changed, 39 deletions(-)
> >
> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >index 2e56af3281f8..579f58a42974 100644
> >--- a/arch/riscv/kernel/setup.c
> >+++ b/arch/riscv/kernel/setup.c
> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
> > /* The lucky hart to first increment this variable will boot the other cores */
> > atomic_t hart_lottery;
> >
> >-#ifdef CONFIG_BLK_DEV_INITRD
> >-static void __init setup_initrd(void)
> >-{
> >-	extern char __initramfs_start[];
> >-	extern unsigned long __initramfs_size;
> >-	unsigned long size;
> >-
> >-	if (__initramfs_size > 0) {
> >-		initrd_start = (unsigned long)(&__initramfs_start);
> >-		initrd_end = initrd_start + __initramfs_size;
> >-	}

The underlying problem is probably that __initramfs_size == 512 even
if there is no embedded initrd. Result is that initrd_start and initrd_end
are always overwritten, even if provided and even if there is no embedded
initrd. Result is that initrd_start and initrd_end are always overwritten,
and -initrd from the qemu command line is always ignored.

A less invasive fix than mine would be

-	if (__initramfs_size > 0) {
+	if (__initramfs_size > 0 && !initrd_start) {

Any chance you can test that with your setup ?

Thanks,
Guenter

> >-
> >-	if (initrd_start >= initrd_end) {
> >-		printk(KERN_INFO "initrd not found or empty");
> >-		goto disable;
> >-	}
> >-	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
> >-		printk(KERN_ERR "initrd extends beyond end of memory");
> >-		goto disable;
> >-	}
> >-
> >-	size =  initrd_end - initrd_start;
> >-	memblock_reserve(__pa(initrd_start), size);
> >-	initrd_below_start_ok = 1;
> >-
> >-	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
> >-		(void *)(initrd_start), size);
> >-	return;
> >-disable:
> >-	pr_cont(" - disabling initrd\n");
> >-	initrd_start = 0;
> >-	initrd_end = 0;
> >-}
> >-#endif /* CONFIG_BLK_DEV_INITRD */
> >-
> > pgd_t swapper_pg_dir[PTRS_PER_PGD] __page_aligned_bss;
> > pgd_t trampoline_pg_dir[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> >
> >@@ -195,10 +160,6 @@ static void __init setup_bootmem(void)
> > 	set_max_mapnr(PFN_DOWN(mem_size));
> > 	max_low_pfn = pfn_base + PFN_DOWN(mem_size);
> >
> >-#ifdef CONFIG_BLK_DEV_INITRD
> >-	setup_initrd();
> >-#endif /* CONFIG_BLK_DEV_INITRD */
> >-
> > 	early_init_fdt_reserve_self();
> > 	early_init_fdt_scan_reserved_mem();
> > 	memblock_allow_resize();
> 
> With this patch I can't boot in QEMU.  It might be something with my setup,
> though.  I'm testing with a unified BBL+vmlinux, and using QEMU 3.0.0-rc3 (I
> should probably update that one, but I don't remember anything going by).
> Do you have time to take a look?  Here's my QEMU commandline
> 
> ./riscv64-softmmu/qemu-system-riscv64    -nographic    -machine virt    -smp 4    -m 2G    -kernel /home/palmer/work/riscv/riscv-pk/build/bbl -object rng-random,filename=/dev/urandom,id=rng0    -device virtio-rng-device,rng=rng0    -append "console=ttyS0 ro root=/dev/vda"    -device virtio-blk-device,drive=hd0    -drive file=stage4-disk.img,format=raw,id=hd0    -device virtio-net-device,netdev=usernet    -netdev user,id=usernet,hostfwd=tcp::10000-:22
> 
> I just tagged the PR I plan to submit for RC2 tomorrow, so at least it
> should be easy for everyone to get on the same page.

Re: [PATCH] riscv: Drop setup_initrd

[Andreas Schwab]

On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:

> On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
>> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
>> >setup_initrd() does not appear to serve a practical purpose other than
>> >preventing qemu boots with "-initrd" parameter, so let's drop it.
>> >
>> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> >---
>> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
>> > 1 file changed, 39 deletions(-)
>> >
>> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> >index 2e56af3281f8..579f58a42974 100644
>> >--- a/arch/riscv/kernel/setup.c
>> >+++ b/arch/riscv/kernel/setup.c
>> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>> > /* The lucky hart to first increment this variable will boot the other cores */
>> > atomic_t hart_lottery;
>> >
>> >-#ifdef CONFIG_BLK_DEV_INITRD
>> >-static void __init setup_initrd(void)
>> >-{
>> >-	extern char __initramfs_start[];
>> >-	extern unsigned long __initramfs_size;
>> >-	unsigned long size;
>> >-
>> >-	if (__initramfs_size > 0) {
>> >-		initrd_start = (unsigned long)(&__initramfs_start);
>> >-		initrd_end = initrd_start + __initramfs_size;
>> >-	}
>
> The underlying problem is probably that __initramfs_size == 512 even
> if there is no embedded initrd. Result is that initrd_start and initrd_end
> are always overwritten, even if provided and even if there is no embedded
> initrd. Result is that initrd_start and initrd_end are always overwritten,
> and -initrd from the qemu command line is always ignored.
>
> A less invasive fix than mine would be
>
> -	if (__initramfs_size > 0) {
> +	if (__initramfs_size > 0 && !initrd_start) {
>
> Any chance you can test that with your setup ?

You should just delete the last four lines above.  They serve no purpose.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
> On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
> >> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> >> >setup_initrd() does not appear to serve a practical purpose other than
> >> >preventing qemu boots with "-initrd" parameter, so let's drop it.
> >> >
> >> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> >---
> >> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
> >> > 1 file changed, 39 deletions(-)
> >> >
> >> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> >index 2e56af3281f8..579f58a42974 100644
> >> >--- a/arch/riscv/kernel/setup.c
> >> >+++ b/arch/riscv/kernel/setup.c
> >> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
> >> > /* The lucky hart to first increment this variable will boot the other cores */
> >> > atomic_t hart_lottery;
> >> >
> >> >-#ifdef CONFIG_BLK_DEV_INITRD
> >> >-static void __init setup_initrd(void)
> >> >-{
> >> >-	extern char __initramfs_start[];
> >> >-	extern unsigned long __initramfs_size;
> >> >-	unsigned long size;
> >> >-
> >> >-	if (__initramfs_size > 0) {
> >> >-		initrd_start = (unsigned long)(&__initramfs_start);
> >> >-		initrd_end = initrd_start + __initramfs_size;
> >> >-	}
> >
> > The underlying problem is probably that __initramfs_size == 512 even
> > if there is no embedded initrd. Result is that initrd_start and initrd_end
> > are always overwritten, even if provided and even if there is no embedded
> > initrd. Result is that initrd_start and initrd_end are always overwritten,
> > and -initrd from the qemu command line is always ignored.
> >
> > A less invasive fix than mine would be
> >
> > -	if (__initramfs_size > 0) {
> > +	if (__initramfs_size > 0 && !initrd_start) {
> >
> > Any chance you can test that with your setup ?
> 
> You should just delete the last four lines above.  They serve no purpose.
> 

You mean the entire if() statement plus the variable declarations ?

That works for me, for both embedded initrd and initrd specified with
-initrd option, but we still need someone to test if it works for 
Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
bbl.

Thanks,
Guenter

> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Re: [PATCH] riscv: Drop setup_initrd

[Palmer Dabbelt]

On Tue, 28 Aug 2018 14:59:59 PDT (-0700), linux@roeck-us.net wrote:
> On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
>> On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> > On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
>> >> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
>> >> >setup_initrd() does not appear to serve a practical purpose other than
>> >> >preventing qemu boots with "-initrd" parameter, so let's drop it.
>> >> >
>> >> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> >> >---
>> >> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
>> >> > 1 file changed, 39 deletions(-)
>> >> >
>> >> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> >> >index 2e56af3281f8..579f58a42974 100644
>> >> >--- a/arch/riscv/kernel/setup.c
>> >> >+++ b/arch/riscv/kernel/setup.c
>> >> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>> >> > /* The lucky hart to first increment this variable will boot the other cores */
>> >> > atomic_t hart_lottery;
>> >> >
>> >> >-#ifdef CONFIG_BLK_DEV_INITRD
>> >> >-static void __init setup_initrd(void)
>> >> >-{
>> >> >-	extern char __initramfs_start[];
>> >> >-	extern unsigned long __initramfs_size;
>> >> >-	unsigned long size;
>> >> >-
>> >> >-	if (__initramfs_size > 0) {
>> >> >-		initrd_start = (unsigned long)(&__initramfs_start);
>> >> >-		initrd_end = initrd_start + __initramfs_size;
>> >> >-	}
>> >
>> > The underlying problem is probably that __initramfs_size == 512 even
>> > if there is no embedded initrd. Result is that initrd_start and initrd_end
>> > are always overwritten, even if provided and even if there is no embedded
>> > initrd. Result is that initrd_start and initrd_end are always overwritten,
>> > and -initrd from the qemu command line is always ignored.
>> >
>> > A less invasive fix than mine would be
>> >
>> > -	if (__initramfs_size > 0) {
>> > +	if (__initramfs_size > 0 && !initrd_start) {
>> >
>> > Any chance you can test that with your setup ?
>>
>> You should just delete the last four lines above.  They serve no purpose.
>>
>
> You mean the entire if() statement plus the variable declarations ?
>
> That works for me, for both embedded initrd and initrd specified with
> -initrd option, but we still need someone to test if it works for
> Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
> bbl.

This still boots my Fedora images

    diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
    index db20dc630e7e..aee603123030 100644
    --- a/arch/riscv/kernel/setup.c
    +++ b/arch/riscv/kernel/setup.c
    @@ -85,15 +85,8 @@ atomic_t hart_lottery;
     #ifdef CONFIG_BLK_DEV_INITRD
     static void __init setup_initrd(void)
     {
    -	extern char __initramfs_start[];
    -	extern unsigned long __initramfs_size;
     	unsigned long size;
     
    -	if (__initramfs_size > 0) {
    -		initrd_start = (unsigned long)(&__initramfs_start);
    -		initrd_end = initrd_start + __initramfs_size;
    -	}
    -
     	if (initrd_start >= initrd_end) {
     		printk(KERN_INFO "initrd not found or empty");
     		goto disable;

but I have not tried an integrated initramfs.

Re: [PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

On Tue, Aug 28, 2018 at 03:03:00PM -0700, Palmer Dabbelt wrote:
> On Tue, 28 Aug 2018 14:59:59 PDT (-0700), linux@roeck-us.net wrote:
> >On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
> >>On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >>> On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
> >>>> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
> >>>> >setup_initrd() does not appear to serve a practical purpose other than
> >>>> >preventing qemu boots with "-initrd" parameter, so let's drop it.
> >>>> >
> >>>> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>> >---
> >>>> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
> >>>> > 1 file changed, 39 deletions(-)
> >>>> >
> >>>> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >>>> >index 2e56af3281f8..579f58a42974 100644
> >>>> >--- a/arch/riscv/kernel/setup.c
> >>>> >+++ b/arch/riscv/kernel/setup.c
> >>>> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
> >>>> > /* The lucky hart to first increment this variable will boot the other cores */
> >>>> > atomic_t hart_lottery;
> >>>> >
> >>>> >-#ifdef CONFIG_BLK_DEV_INITRD
> >>>> >-static void __init setup_initrd(void)
> >>>> >-{
> >>>> >-	extern char __initramfs_start[];
> >>>> >-	extern unsigned long __initramfs_size;
> >>>> >-	unsigned long size;
> >>>> >-
> >>>> >-	if (__initramfs_size > 0) {
> >>>> >-		initrd_start = (unsigned long)(&__initramfs_start);
> >>>> >-		initrd_end = initrd_start + __initramfs_size;
> >>>> >-	}
> >>>
> >>> The underlying problem is probably that __initramfs_size == 512 even
> >>> if there is no embedded initrd. Result is that initrd_start and initrd_end
> >>> are always overwritten, even if provided and even if there is no embedded
> >>> initrd. Result is that initrd_start and initrd_end are always overwritten,
> >>> and -initrd from the qemu command line is always ignored.
> >>>
> >>> A less invasive fix than mine would be
> >>>
> >>> -	if (__initramfs_size > 0) {
> >>> +	if (__initramfs_size > 0 && !initrd_start) {
> >>>
> >>> Any chance you can test that with your setup ?
> >>
> >>You should just delete the last four lines above.  They serve no purpose.
> >>
> >
> >You mean the entire if() statement plus the variable declarations ?
> >
> >That works for me, for both embedded initrd and initrd specified with
> >-initrd option, but we still need someone to test if it works for
> >Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
> >bbl.
> 
> This still boots my Fedora images
> 
>    diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>    index db20dc630e7e..aee603123030 100644
>    --- a/arch/riscv/kernel/setup.c
>    +++ b/arch/riscv/kernel/setup.c
>    @@ -85,15 +85,8 @@ atomic_t hart_lottery;
>     #ifdef CONFIG_BLK_DEV_INITRD
>     static void __init setup_initrd(void)
>     {
>    -	extern char __initramfs_start[];
>    -	extern unsigned long __initramfs_size;
>     	unsigned long size;
>    -	if (__initramfs_size > 0) {
>    -		initrd_start = (unsigned long)(&__initramfs_start);
>    -		initrd_end = initrd_start + __initramfs_size;
>    -	}
>    -
>     	if (initrd_start >= initrd_end) {
>     		printk(KERN_INFO "initrd not found or empty");
>     		goto disable;
> 
> but I have not tried an integrated initramfs.

I tested the above both with external initrd and with integrated
initramfs; both work for me.

Should I resend my patch, using the above, or do you want to create
a patch yourself ?

Thanks,
Guenter

Re: [PATCH] riscv: Drop setup_initrd

[Palmer Dabbelt]

On Tue, 28 Aug 2018 15:12:38 PDT (-0700), linux@roeck-us.net wrote:
> On Tue, Aug 28, 2018 at 03:03:00PM -0700, Palmer Dabbelt wrote:
>> On Tue, 28 Aug 2018 14:59:59 PDT (-0700), linux@roeck-us.net wrote:
>> >On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
>> >>On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
>> >>
>> >>> On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
>> >>>> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
>> >>>> >setup_initrd() does not appear to serve a practical purpose other than
>> >>>> >preventing qemu boots with "-initrd" parameter, so let's drop it.
>> >>>> >
>> >>>> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> >>>> >---
>> >>>> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
>> >>>> > 1 file changed, 39 deletions(-)
>> >>>> >
>> >>>> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> >>>> >index 2e56af3281f8..579f58a42974 100644
>> >>>> >--- a/arch/riscv/kernel/setup.c
>> >>>> >+++ b/arch/riscv/kernel/setup.c
>> >>>> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>> >>>> > /* The lucky hart to first increment this variable will boot the other cores */
>> >>>> > atomic_t hart_lottery;
>> >>>> >
>> >>>> >-#ifdef CONFIG_BLK_DEV_INITRD
>> >>>> >-static void __init setup_initrd(void)
>> >>>> >-{
>> >>>> >-	extern char __initramfs_start[];
>> >>>> >-	extern unsigned long __initramfs_size;
>> >>>> >-	unsigned long size;
>> >>>> >-
>> >>>> >-	if (__initramfs_size > 0) {
>> >>>> >-		initrd_start = (unsigned long)(&__initramfs_start);
>> >>>> >-		initrd_end = initrd_start + __initramfs_size;
>> >>>> >-	}
>> >>>
>> >>> The underlying problem is probably that __initramfs_size == 512 even
>> >>> if there is no embedded initrd. Result is that initrd_start and initrd_end
>> >>> are always overwritten, even if provided and even if there is no embedded
>> >>> initrd. Result is that initrd_start and initrd_end are always overwritten,
>> >>> and -initrd from the qemu command line is always ignored.
>> >>>
>> >>> A less invasive fix than mine would be
>> >>>
>> >>> -	if (__initramfs_size > 0) {
>> >>> +	if (__initramfs_size > 0 && !initrd_start) {
>> >>>
>> >>> Any chance you can test that with your setup ?
>> >>
>> >>You should just delete the last four lines above.  They serve no purpose.
>> >>
>> >
>> >You mean the entire if() statement plus the variable declarations ?
>> >
>> >That works for me, for both embedded initrd and initrd specified with
>> >-initrd option, but we still need someone to test if it works for
>> >Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
>> >bbl.
>>
>> This still boots my Fedora images
>>
>>    diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>    index db20dc630e7e..aee603123030 100644
>>    --- a/arch/riscv/kernel/setup.c
>>    +++ b/arch/riscv/kernel/setup.c
>>    @@ -85,15 +85,8 @@ atomic_t hart_lottery;
>>     #ifdef CONFIG_BLK_DEV_INITRD
>>     static void __init setup_initrd(void)
>>     {
>>    -	extern char __initramfs_start[];
>>    -	extern unsigned long __initramfs_size;
>>     	unsigned long size;
>>    -	if (__initramfs_size > 0) {
>>    -		initrd_start = (unsigned long)(&__initramfs_start);
>>    -		initrd_end = initrd_start + __initramfs_size;
>>    -	}
>>    -
>>     	if (initrd_start >= initrd_end) {
>>     		printk(KERN_INFO "initrd not found or empty");
>>     		goto disable;
>>
>> but I have not tried an integrated initramfs.
>
> I tested the above both with external initrd and with integrated
> initramfs; both work for me.
>
> Should I resend my patch, using the above, or do you want to create
> a patch yourself ?

You should send one, then it'll go through my regular pre-PR testing flow to 
make sure it works on my end.  I just never trust these inline patches to be 
exact, even if it's unlikely there's any confusion on a patch this simple (at 
least, mechanically simple -- I'm afraid I still don't understand why the logic 
is incorrect).

Re: [PATCH] riscv: Drop setup_initrd

[Guenter Roeck]

On 08/28/2018 05:09 PM, Palmer Dabbelt wrote:
> On Tue, 28 Aug 2018 15:12:38 PDT (-0700), linux@roeck-us.net wrote:
>> On Tue, Aug 28, 2018 at 03:03:00PM -0700, Palmer Dabbelt wrote:
>>> On Tue, 28 Aug 2018 14:59:59 PDT (-0700), linux@roeck-us.net wrote:
>>> >On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
>>> >>On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
>>> >>
>>> >>> On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
>>> >>>> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
>>> >>>> >setup_initrd() does not appear to serve a practical purpose other than
>>> >>>> >preventing qemu boots with "-initrd" parameter, so let's drop it.
>>> >>>> >
>>> >>>> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> >>>> >---
>>> >>>> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
>>> >>>> > 1 file changed, 39 deletions(-)
>>> >>>> >
>>> >>>> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> >>>> >index 2e56af3281f8..579f58a42974 100644
>>> >>>> >--- a/arch/riscv/kernel/setup.c
>>> >>>> >+++ b/arch/riscv/kernel/setup.c
>>> >>>> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>>> >>>> > /* The lucky hart to first increment this variable will boot the other cores */
>>> >>>> > atomic_t hart_lottery;
>>> >>>> >
>>> >>>> >-#ifdef CONFIG_BLK_DEV_INITRD
>>> >>>> >-static void __init setup_initrd(void)
>>> >>>> >-{
>>> >>>> >-    extern char __initramfs_start[];
>>> >>>> >-    extern unsigned long __initramfs_size;
>>> >>>> >-    unsigned long size;
>>> >>>> >-
>>> >>>> >-    if (__initramfs_size > 0) {
>>> >>>> >-        initrd_start = (unsigned long)(&__initramfs_start);
>>> >>>> >-        initrd_end = initrd_start + __initramfs_size;
>>> >>>> >-    }
>>> >>>
>>> >>> The underlying problem is probably that __initramfs_size == 512 even
>>> >>> if there is no embedded initrd. Result is that initrd_start and initrd_end
>>> >>> are always overwritten, even if provided and even if there is no embedded
>>> >>> initrd. Result is that initrd_start and initrd_end are always overwritten,
>>> >>> and -initrd from the qemu command line is always ignored.
>>> >>>
>>> >>> A less invasive fix than mine would be
>>> >>>
>>> >>> -    if (__initramfs_size > 0) {
>>> >>> +    if (__initramfs_size > 0 && !initrd_start) {
>>> >>>
>>> >>> Any chance you can test that with your setup ?
>>> >>
>>> >>You should just delete the last four lines above.  They serve no purpose.
>>> >>
>>> >
>>> >You mean the entire if() statement plus the variable declarations ?
>>> >
>>> >That works for me, for both embedded initrd and initrd specified with
>>> >-initrd option, but we still need someone to test if it works for
>>> >Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
>>> >bbl.
>>>
>>> This still boots my Fedora images
>>>
>>>    diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>    index db20dc630e7e..aee603123030 100644
>>>    --- a/arch/riscv/kernel/setup.c
>>>    +++ b/arch/riscv/kernel/setup.c
>>>    @@ -85,15 +85,8 @@ atomic_t hart_lottery;
>>>     #ifdef CONFIG_BLK_DEV_INITRD
>>>     static void __init setup_initrd(void)
>>>     {
>>>    -    extern char __initramfs_start[];
>>>    -    extern unsigned long __initramfs_size;
>>>         unsigned long size;
>>>    -    if (__initramfs_size > 0) {
>>>    -        initrd_start = (unsigned long)(&__initramfs_start);
>>>    -        initrd_end = initrd_start + __initramfs_size;
>>>    -    }
>>>    -
>>>         if (initrd_start >= initrd_end) {
>>>             printk(KERN_INFO "initrd not found or empty");
>>>             goto disable;
>>>
>>> but I have not tried an integrated initramfs.
>>
>> I tested the above both with external initrd and with integrated
>> initramfs; both work for me.
>>
>> Should I resend my patch, using the above, or do you want to create
>> a patch yourself ?
> 
> You should send one, then it'll go through my regular pre-PR testing flow to make sure it works on my end.  I just never trust these inline patches to be exact, even if it's unlikely there's any confusion on a patch this simple (at least, mechanically simple -- I'm afraid I still don't understand why the logic is incorrect).
> 

Done.

There is no need to override initrd_start and initrd_end; populate_rootfs()
uses __initramfs_start and __initramfs_size directly when loading a built-in
initramfs. initrd_start and initrd_end, on the other side, are for an external
initrd, loaded separately (and initialized in __early_init_dt_declare_initrd()).

Hope this helps,
Guenter

Re: [PATCH] riscv: Drop setup_initrd

[Palmer Dabbelt]

On Tue, 28 Aug 2018 17:36:28 PDT (-0700), linux@roeck-us.net wrote:
> On 08/28/2018 05:09 PM, Palmer Dabbelt wrote:
>> On Tue, 28 Aug 2018 15:12:38 PDT (-0700), linux@roeck-us.net wrote:
>>> On Tue, Aug 28, 2018 at 03:03:00PM -0700, Palmer Dabbelt wrote:
>>>> On Tue, 28 Aug 2018 14:59:59 PDT (-0700), linux@roeck-us.net wrote:
>>>> >On Tue, Aug 28, 2018 at 11:46:09PM +0200, Andreas Schwab wrote:
>>>> >>On Aug 28 2018, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> >>
>>>> >>> On Tue, Aug 28, 2018 at 01:10:20PM -0700, Palmer Dabbelt wrote:
>>>> >>>> On Thu, 09 Aug 2018 21:11:40 PDT (-0700), linux@roeck-us.net wrote:
>>>> >>>> >setup_initrd() does not appear to serve a practical purpose other than
>>>> >>>> >preventing qemu boots with "-initrd" parameter, so let's drop it.
>>>> >>>> >
>>>> >>>> >Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> >>>> >---
>>>> >>>> > arch/riscv/kernel/setup.c | 39 ---------------------------------------
>>>> >>>> > 1 file changed, 39 deletions(-)
>>>> >>>> >
>>>> >>>> >diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>> >>>> >index 2e56af3281f8..579f58a42974 100644
>>>> >>>> >--- a/arch/riscv/kernel/setup.c
>>>> >>>> >+++ b/arch/riscv/kernel/setup.c
>>>> >>>> >@@ -82,41 +82,6 @@ EXPORT_SYMBOL(empty_zero_page);
>>>> >>>> > /* The lucky hart to first increment this variable will boot the other cores */
>>>> >>>> > atomic_t hart_lottery;
>>>> >>>> >
>>>> >>>> >-#ifdef CONFIG_BLK_DEV_INITRD
>>>> >>>> >-static void __init setup_initrd(void)
>>>> >>>> >-{
>>>> >>>> >-    extern char __initramfs_start[];
>>>> >>>> >-    extern unsigned long __initramfs_size;
>>>> >>>> >-    unsigned long size;
>>>> >>>> >-
>>>> >>>> >-    if (__initramfs_size > 0) {
>>>> >>>> >-        initrd_start = (unsigned long)(&__initramfs_start);
>>>> >>>> >-        initrd_end = initrd_start + __initramfs_size;
>>>> >>>> >-    }
>>>> >>>
>>>> >>> The underlying problem is probably that __initramfs_size == 512 even
>>>> >>> if there is no embedded initrd. Result is that initrd_start and initrd_end
>>>> >>> are always overwritten, even if provided and even if there is no embedded
>>>> >>> initrd. Result is that initrd_start and initrd_end are always overwritten,
>>>> >>> and -initrd from the qemu command line is always ignored.
>>>> >>>
>>>> >>> A less invasive fix than mine would be
>>>> >>>
>>>> >>> -    if (__initramfs_size > 0) {
>>>> >>> +    if (__initramfs_size > 0 && !initrd_start) {
>>>> >>>
>>>> >>> Any chance you can test that with your setup ?
>>>> >>
>>>> >>You should just delete the last four lines above.  They serve no purpose.
>>>> >>
>>>> >
>>>> >You mean the entire if() statement plus the variable declarations ?
>>>> >
>>>> >That works for me, for both embedded initrd and initrd specified with
>>>> >-initrd option, but we still need someone to test if it works for
>>>> >Palmer's use case, ie with vmlinux (and possibly initrd) embedded in
>>>> >bbl.
>>>>
>>>> This still boots my Fedora images
>>>>
>>>>    diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>>    index db20dc630e7e..aee603123030 100644
>>>>    --- a/arch/riscv/kernel/setup.c
>>>>    +++ b/arch/riscv/kernel/setup.c
>>>>    @@ -85,15 +85,8 @@ atomic_t hart_lottery;
>>>>     #ifdef CONFIG_BLK_DEV_INITRD
>>>>     static void __init setup_initrd(void)
>>>>     {
>>>>    -    extern char __initramfs_start[];
>>>>    -    extern unsigned long __initramfs_size;
>>>>         unsigned long size;
>>>>    -    if (__initramfs_size > 0) {
>>>>    -        initrd_start = (unsigned long)(&__initramfs_start);
>>>>    -        initrd_end = initrd_start + __initramfs_size;
>>>>    -    }
>>>>    -
>>>>         if (initrd_start >= initrd_end) {
>>>>             printk(KERN_INFO "initrd not found or empty");
>>>>             goto disable;
>>>>
>>>> but I have not tried an integrated initramfs.
>>>
>>> I tested the above both with external initrd and with integrated
>>> initramfs; both work for me.
>>>
>>> Should I resend my patch, using the above, or do you want to create
>>> a patch yourself ?
>>
>> You should send one, then it'll go through my regular pre-PR testing flow to make sure it works on my end.  I just never trust these inline patches to be exact, even if it's unlikely there's any confusion on a patch this simple (at least, mechanically simple -- I'm afraid I still don't understand why the logic is incorrect).
>>
>
> Done.
>
> There is no need to override initrd_start and initrd_end; populate_rootfs()
> uses __initramfs_start and __initramfs_size directly when loading a built-in
> initramfs. initrd_start and initrd_end, on the other side, are for an external
> initrd, loaded separately (and initialized in __early_init_dt_declare_initrd()).

OK, that makes sense.  I dropped your patch onto for-next, assuming nothing's 
gone wrong it should go in next week.  It boots a Fedora rootfs for me.

Thanks!