Re: [PATCH master 1/3] RISC-V: cpu: request stack memory region

[Antony Pavlov <antonynpavlov@gmail.com>]

On Fri, 2 Apr 2021 08:34:36 +0200
Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

Hi Ahmad!

> Hello Antony,
> 
> On 02.04.21 07:56, Antony Pavlov wrote:
> > On Wed, 24 Mar 2021 09:23:02 +0100
> > Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > 
> > Current RISC-V erizo barebox master succesfully runs on QEMU
> > but hangs on FPGA.
> > 
> > git bisection shows this:
> > 
> >     barebox$ git bisect good 809e66d4ef
> >     ...
> >     barebox$ git bisect bad
> >     fef19e17f651a7f4b6063a76c506a67cabfe4a69 is the first bad commit
> >     commit fef19e17f651a7f4b6063a76c506a67cabfe4a69
> >     Author: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >     Date:   Wed Mar 24 09:23:02 2021 +0100
> > 
> >         RISC-V: cpu: request stack memory region
> > 
> > I see that request_sdram_region("stack", ...) hangs in input_data_len().
> > 
> > On FPGA I load barebox image at 0x80000000 (erizo RAM start)
> > not at 0x0 (link address).
> > The problem is that input_data_end in input_data_len()
> > contains link-time address, not actual run-time address.
> 
> This is strange. Could you print get_runtime_offset() at this location?
> It should be zero, because relocate_to_current_adr() had previously run,
> but it seems it's not for you? I don't see how compiler reordering could have
> occurred given that we don't do LTO and relocate_to_current_adr is thus
> a compiler barrier.

I have added debug output:

--- a/arch/riscv/boot/uncompress.c
+++ b/arch/riscv/boot/uncompress.c
@@ -46,6 +46,9 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
                relocate_to_adr(membase);
 
        pg_len = pg_end - pg_start;
+       puts_ll("get_runtime_offset()=0x");
+       puthex_ll(get_runtime_offset());
+       puts_ll("\n");
        uncompressed_len = input_data_len();
 
        barebox_base = riscv_mem_barebox_image(membase, endmem,


I see 

  >get_runtime_offset()=0x00000000

on both QEMU and FPGA.


I have experimented with empty_slot on qemu. To reproduce my result please
apply this patch to erizo qemu, reconfigure and rebuild qemu.

diff --git a/default-configs/riscv32-softmmu.mak b/default-configs/riscv32-softmmu.mak
index f9e742120c..5aff9ed690 100644
--- a/default-configs/riscv32-softmmu.mak
+++ b/default-configs/riscv32-softmmu.mak
@@ -2,3 +2,4 @@
 
 CONFIG_SERIAL=y
 CONFIG_VIRTIO=y
+CONFIG_EMPTY_SLOT=y
diff --git a/default-configs/riscv64-softmmu.mak b/default-configs/riscv64-softmmu.mak
index f9e742120c..5aff9ed690 100644
--- a/default-configs/riscv64-softmmu.mak
+++ b/default-configs/riscv64-softmmu.mak
@@ -2,3 +2,4 @@
 
 CONFIG_SERIAL=y
 CONFIG_VIRTIO=y
+CONFIG_EMPTY_SLOT=y
diff --git a/hw/core/empty_slot.c b/hw/core/empty_slot.c
index c1b9c2b104..2358b849e8 100644
--- a/hw/core/empty_slot.c
+++ b/hw/core/empty_slot.c
@@ -14,7 +14,7 @@
 #include "hw/sysbus.h"
 #include "hw/empty_slot.h"
 
-//#define DEBUG_EMPTY_SLOT
+#define DEBUG_EMPTY_SLOT
 
 #ifdef DEBUG_EMPTY_SLOT
 #define DPRINTF(fmt, ...)                                       \
diff --git a/hw/riscv/erizo_board.c b/hw/riscv/erizo_board.c
index 9586c4df2f..c34f94cedc 100644
--- a/hw/riscv/erizo_board.c
+++ b/hw/riscv/erizo_board.c
@@ -17,6 +17,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/boards.h"
+#include "hw/empty_slot.h"
 #include "hw/riscv/riscv_hart.h"
 #include "hw/char/serial.h"
 #include "sysemu/arch_init.h"
@@ -66,6 +67,8 @@ static void erizo_init(MachineState *machine)
     DeviceState *dev = qdev_create(NULL, TYPE_ERIZO);
     ErizoState *s = ERIZO(dev);
 
+    empty_slot_init(0, 0xffffffff);
+
     qdev_init_nofail(dev);
     harts = &s->harts;
 
There are empty_slot read warnings on master:

    barebox$ qemu-system-riscv32 -nographic -M erizo -bios ./images/barebox-erizo-generic.img -serial stdio -monitor none -trace file=/dev/null 
    >get_runtime_offset()=0x00000000
    empty_slot: read from 0000000000022caf
    empty_slot: read from 0000000000022cae
    empty_slot: read from 0000000000022cb0
    empty_slot: read from 0000000000022cb1
    riscv-timer riscv-timer: running at 24000000 Hz
    Switch to console [cs0]

    barebox 2021.03.0-00134-gc4a1684da9-dirty #1 Fri Apr 2 10:33:45 MSK 2021

    Board: generic Erizo SoC board
    riscv-timer riscv-timer: running at 24000000 Hz
    malloc space: 0x804ffd80 -> 0x805ffd7f (size 1 MiB)

    Hit any to stop autoboot:    1
    boot: error 2
    barebox:/ 


There is no empty_slot warning with fef19e17f651^ barebox:

    barebox$ qemu-system-riscv32 -nographic -M erizo -bios ./images/barebox-erizo-generic.img -serial stdio -monitor none -trace file=/dev/null 
    >RISC-V system with no 'timebase-frequency' in DTS
    Switch to console [cs0]

    barebox 2021.03.0-00122-g6107208573 #1 Fri Apr 2 11:09:16 MSK 2021

    Board: generic Erizo SoC board
    RISC-V system with no 'timebase-frequency' in DTS
    malloc space: 0x802ffd80 -> 0x803ffd7f (size 1 MiB)

    Hit any to stop autoboot:    0
    boot: error 2
    barebox:/ 





> Thanks for testing,
> Ahmad
> 
> > 
> > I have added this hack
> > 
> > --- a/arch/riscv/boot/uncompress.c
> > +++ b/arch/riscv/boot/uncompress.c
> > @@ -46,7 +46,7 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
> >                 relocate_to_adr(membase);
> >  
> >         pg_len = pg_end - pg_start;
> > -       uncompressed_len = input_data_len();
> > +       uncompressed_len = get_unaligned((const u32 *)(input_data_end - 4 + get_runtime_offset()));
> >  
> >         barebox_base = riscv_mem_barebox_image(membase, endmem,
> >                                                uncompressed_len + MAX_BSS_SIZE);
> > 
> > Alas, this hack does not fix the problem completely.
> > 
> > 
> >> Now that the stack base region is determined dynamically,
> >> mem_malloc_resource can no longer reserve the stack space.
> >> Do as ARM does and add a RISC-V specific initcall to reserve
> >> the main thread's stack space.
> >>
> >> Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> Fix for master as otherwise stack could be overwritten at runtime
> >> ---
> >>  arch/riscv/cpu/core.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/arch/riscv/cpu/core.c b/arch/riscv/cpu/core.c
> >> index bdcd500ed748..982d378eddec 100644
> >> --- a/arch/riscv/cpu/core.c
> >> +++ b/arch/riscv/cpu/core.c
> >> @@ -2,6 +2,9 @@
> >>  /*
> >>   * Copyright (C) 2012 Regents of the University of California
> >>   * Copyright (C) 2017 SiFive
> >> + * Copyright (C) 2021 Ahmad Fatoum, Pengutronix
> >> + *
> >> + * Common RISC-V core initcalls.
> >>   *
> >>   * All RISC-V systems have a timer attached to every hart.  These timers can
> >>   * either be read from the "time" and "timeh" CSRs, and can use the SBI to
> >> @@ -14,8 +17,17 @@
> >>  #include <of.h>
> >>  #include <linux/clk.h>
> >>  #include <linux/err.h>
> >> +#include <memory.h>
> >> +#include <asm-generic/memory_layout.h>
> >>  #include <io.h>
> >>  
> >> +static int riscv_request_stack(void)
> >> +{
> >> +	extern unsigned long riscv_stack_top;
> >> +	return PTR_ERR_OR_ZERO(request_sdram_region("stack", riscv_stack_top - STACK_SIZE, STACK_SIZE));
> >> +}
> >> +coredevice_initcall(riscv_request_stack);
> >> +
> >>  static struct device_d timer_dev;
> >>  
> >>  static int riscv_probe(struct device_d *parent)
> >> -- 
> >> 2.29.2
> >>
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox