Re: [PATCH v2 7/8] leon3: check cpu_id in the tiny bootloader

[Clément Chigot]

On Tue, Jan 30, 2024 at 10:15 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 16/1/24 14:02, Clément Chigot wrote:
> > Now that SMP is possible, the asr17 must be checked in the little boot code
> > or the secondary CPU will reinitialize the Timer and the Uart.
> >
> > Co-developed-by: Frederic Konrad <konrad.frederic@yahoo.fr>
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > ---
> >   hw/sparc/leon3.c | 22 ++++++++++++++++++++--
> >   1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> > index eacd85ee4f..87a8044a3e 100644
> > --- a/hw/sparc/leon3.c
> > +++ b/hw/sparc/leon3.c
> > @@ -100,13 +100,27 @@ static uint32_t *gen_store_u32(uint32_t *code, hwaddr 
> > addr, uint32_t val)
> >
> >   /*
> >    * When loading a kernel in RAM the machine is expected to be in a 
> > different
> > - * state (eg: initialized by the bootloader). This little code reproduces
> > - * this behavior.
> > + * state (eg: initialized by the bootloader).  This little code reproduces
> > + * this behavior.  Also this code can be executed by the secondary cpus as
> > + * well since it looks at the %asr17 register before doing any
> > + * initialization, it allows to use the same reset address for all the
> > + * cpus.
> >    */
> >   static void write_bootloader(CPUSPARCState *env, uint8_t *base,
> >                                hwaddr kernel_addr)
> >   {
> >       uint32_t *p = (uint32_t *) base;
> > +    uint32_t *sec_cpu_branch_p = NULL;
> > +
> > +    /* If we are running on a secondary CPU, jump directly to the kernel.  
> > */
> > +
> > +    stl_p(p++, 0x85444000); /* rd %asr17, %g2      */
> > +    stl_p(p++, 0x8530a01c); /* srl  %g2, 0x1c, %g2 */
> > +    stl_p(p++, 0x80908000); /* tst  %g2            */
> > +    /* Fill that later.  */
>
>        /* Filled below. */
>
> > +    sec_cpu_branch_p = p;
> > +    stl_p(p++, 0x0BADC0DE); /* bne xxx             */
> > +    stl_p(p++, 0x01000000); /* nop */
> >
> >       /* Initialize the UARTs                                        */
> >       /* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
> > @@ -120,6 +134,10 @@ static void write_bootloader(CPUSPARCState *env, 
> > uint8_t *base,
> >       /* *GPTIMER0_CONFIG = GPTIMER_ENABLE | GPTIMER_RESTART;        */
> >       p = gen_store_u32(p, 0x80000318, 3);
> >
> > +    /* Now, the relative branch above can be computed.  */
> > +    stl_p(sec_cpu_branch_p, 0x12800000
> > +          + (p - sec_cpu_branch_p));
> > +
> >       /* JUMP to the entry point                                     */
> >       stl_p(p++, 0x82100000); /* mov %g0, %g1 */
> >       stl_p(p++, 0x03000000 + extract32(kernel_addr, 10, 22));
>
> Alternatively have main_cpu_reset / secondary_cpu_reset handlers.
> You could split BL in HWINIT / JUMP, have HWINIT() return # instr
> used and adjust secondary_cpu_reset entry.

Indeed that would make this code a bit easier to maintain. I'll keep
it for a future MR if you don't mind.
Thanks for the idea anyway.

> Anyway,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>