Re: [PATCH 1/2] hw/openrisc/openrisc_sim: keep serial@90000000 as default

[Stafford Horne]

On Mon, Nov 25, 2024 at 02:02:35PM +0000, Peter Maydell wrote:
> On Sat, 23 Nov 2024 at 10:39, Stafford Horne <shorne@gmail.com> wrote:
> >
> > From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >
> > We used to only have a single UART on the platform and it was located at
> > address 0x90000000. When the number of UARTs was increased to 4, the
> > first UART remained at it's location, but instead of being the first one
> > to be registered, it became the last.
> >
> > This caused QEMU to pick 0x90000300 as the default UART, which broke
> > software that hardcoded the address of 0x90000000 and expected it's
> > output to be visible when the user configured only a single console.
> >
> > This caused regressions[1] in the barebox test suite when updating to a
> > newer QEMU. As there seems to be no good reason to register the UARTs in
> > inverse order, let's register them by ascending address, so existing
> > software can remain oblivious to the additional UART ports.
> >
> > Changing the order of uart registration alone breaks Linux which
> > was choosing the UART at 0x90000300 as the default for ttyS0.  To fix
> > Linux we fix two things in the device tree:
> >
> >  1. Define stdout-path only one time for the first registerd UART
> 
> "registered"

OK

> >     instead of incorrectly defining for each UART.
> >  2. Change the UART alias name from 'uart0' to 'serial0' as almost all
> >     Linux tty drivers look for an alias starting with "serial".
> 
> I would recommend for maximum backwards compatibility also changing
> one other thing. With this patch, the UARTs are listed in the
> device tree starting with the one with the highest address and
> working down. You can see this if you run
>  qemu-system-or1k -M or1k-sim -machine dumpdtb=or1k.dtb -kernel /dev/null
> and then
>  dtc -I dtb -O dts /or1k.dtb |less
> -- the output shows that "serial@90000300" is first.
> 
> This happens because (due to an implementation quirk that I forget
> the details of) nodes we add to the DTB in QEMU end up being listed
> in reverse order of creation. I would recommend making the
> UART-creation loop in openrisc_sim_init() run backwards rather
> than forwards, so that the nodes end up in the DTB in ascending order.
>
> This should not affect any guests that do the "right thing" for
> finding their UART, i.e. look at stdout-path or at the UART alias
> nodes; but for Arm we found that at least some guest code had been
> written to just find the first UART node in the dtb and use that.
> 
> (I suspect, incidentally, that this is the reason why 777784bda468
> was using "serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1)" -- it was
> trying to fix this but didn't quite put the change in the right place.)
> 
> That would correspond to squashing in this change on top of your patch:
> 
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -329,11 +329,22 @@ static void openrisc_sim_init(MachineState *machine)
>                                  smp_cpus, cpus, OR1KSIM_OMPIC_IRQ);
>      }
> 
> -    for (n = 0; n < OR1KSIM_UART_COUNT; ++n)
> +    /*
> +     * We create the UART nodes starting with the highest address and
> +     * working downwards, because in QEMU the DTB nodes end up in the
> +     * DTB in reverse order of creation. Correctly-written guest software
> +     * will not care about the node order (it will look at stdout-path
> +     * or the alias nodes), but for the benefit of guest software which
> +     * just looks for the first UART node in the DTB, make sure the
> +     * lowest-address UART (which is QEMU's first serial port) appears
> +     * first in the DTB.
> +     */
> +    for (n = OR1KSIM_UART_COUNT - 1; n >= 0; n--) {
>          openrisc_sim_serial_init(state, or1ksim_memmap[OR1KSIM_UART].base +
>                                          or1ksim_memmap[OR1KSIM_UART].size * 
> n,
>                                   or1ksim_memmap[OR1KSIM_UART].size,
>                                   smp_cpus, cpus, OR1KSIM_UART_IRQ, n);
> +    }
> 
>      load_addr = openrisc_load_kernel(ram_size, kernel_filename,
>                                       &boot_info.bootstrap_pc);

OK it makes sense, I will add this as well.

> 
> > [1]: 
> > https://lore.barebox.org/barebox/707e7c50-aad1-4459-8796-0cc54bab32e2@pengutronix.de/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
> >
> > Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > [stafford: Change to serial0 alias and update change message]
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >  hw/openrisc/openrisc_sim.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> > index 9fb63515ef..5ec9172ccf 100644
> > --- a/hw/openrisc/openrisc_sim.c
> > +++ b/hw/openrisc/openrisc_sim.c
> > @@ -250,7 +250,7 @@ static void openrisc_sim_serial_init(Or1ksimState 
> > *state, hwaddr base,
> >      void *fdt = state->fdt;
> >      char *nodename;
> >      qemu_irq serial_irq;
> > -    char alias[sizeof("uart0")];
> > +    char alias[sizeof("serial0")];
> 
> Using g_strdup_printf() (and a g_autofree pointer) is better than
> a fixed-size array; but I guess we don't really need to clean
> that up in this patch.

I will leave this as is.

> >      int i;
> >
> >      if (num_cpus > 1) {
> > @@ -265,7 +265,7 @@ static void openrisc_sim_serial_init(Or1ksimState 
> > *state, hwaddr base,
> >          serial_irq = get_cpu_irq(cpus, 0, irq_pin);
> >      }
> >      serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200,
> > -                   serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1),
> > +                   serial_hd(uart_idx),
> >                     DEVICE_NATIVE_ENDIAN);
> >
> >      /* Add device tree node for serial. */
> > @@ -277,10 +277,13 @@ static void openrisc_sim_serial_init(Or1ksimState 
> > *state, hwaddr base,
> >      qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", 
> > OR1KSIM_CLK_MHZ);
> >      qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
> >
> > -    /* The /chosen node is created during fdt creation. */
> > -    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> > -    snprintf(alias, sizeof(alias), "uart%d", uart_idx);
> > +    if (uart_idx == 0) {
> > +        /* The /chosen node is created during fdt creation. */
> > +        qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> > +    }
> > +    snprintf(alias, sizeof(alias), "serial%d", uart_idx);
> >      qemu_fdt_setprop_string(fdt, "/aliases", alias, nodename);
> > +
> >      g_free(nodename);
> >  }
> 
> thanks

Thanks,

-Stafford