Re: [kvm-unit-tests PATCH 1/2] riscv: Allow SBI_CONSOLE with no uart in device tree

[Andrew Jones <andrew.jones@linux.dev>]

On Fri, Jun 13, 2025 at 05:11:19AM -0700, Jesse Taube wrote:
> On Fri, Jun 13, 2025 at 12:53 AM Andrew Jones <andrew.jones@linux.dev> wrote:
> >
> > On Thu, Jun 12, 2025 at 01:07:47PM -0700, Jesse Taube wrote:
> > > When CONFIG_SBI_CONSOLE is enabled and there is no uart defined in the
> > > device tree kvm-unit-tests fails to start.
> > >
> > > Only check if uart exists in device tree if SBI_CONSOLE is false.
> > >
> > > Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> > > ---
> > >  lib/riscv/io.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/riscv/io.c b/lib/riscv/io.c
> > > index fb40adb7..96a3c048 100644
> > > --- a/lib/riscv/io.c
> > > +++ b/lib/riscv/io.c
> > > @@ -104,6 +104,7 @@ static void uart0_init_acpi(void)
> > >
> > >  void io_init(void)
> > >  {
> > > +#ifndef CONFIG_SBI_CONSOLE
> > >       if (dt_available())
> > >               uart0_init_fdt();
> > >       else
> > > @@ -114,6 +115,7 @@ void io_init(void)
> > >                      "Found uart at %p, but early base is %p.\n",
> > >                      uart0_base, UART_EARLY_BASE);
> > >       }
> > > +#endif
> >
> > Doesn't this generate uart0_init_fdt/acpi defined but not used types of
> > warnings?
> 
> No, but it should have, I'll fix it.
> 
> > We need to put everything unused under the #ifndef
> > CONFIG_SBI_CONSOLE, just as uart0_read/write already are. Alternatively,
> > we can keep everything out of the #ifndef and export.
> 
> The problem is uart0_init_fdt panics if no compatible uart was found.

Let's do

#ifndef CONFIG_SBI_CONSOLE
   if (ret) {
       printf("%s: Compatible uart not found in the device tree, aborting...\n",__func__);
       abort();
   }
#else
   uart0_base = NULL;
   return;
#endif

And then in uart0_puts() do a 

  assert(uart0_base);

This rules out mapping a uart at physical address zero, but hopefully
that's not a thing. If it ever does appear to be a thing, then we can
just add a boolean to track whether the base address is valid or not.
But we'll burn that bridge when we cross it later, if ever.

Thanks,
drew

> 
> Thanks,
> Jesse Taube
> > void sbi_puts(const char *s);
> > void uart0_puts(const char *s);
> >
> > then just have a single use of the #ifdef,
> >
> > void puts(const char *s)
> > {
> > #ifdef CONFIG_SBI_CONSOLE
> >     sbi_puts(s);
> > #else
> >     uart0_puts(s);
> > #endif
> > }
> >
> > I think I like that better since it will ensure all code gets compile
> > tested all the time and allow an SBI console using unit test to also
> > access its uart if it has one, just because...
> >
> > Thanks,
> > drew
> >
> >
> > >  }
> > >
> > >  #ifdef CONFIG_SBI_CONSOLE
> > > --
> > > 2.43.0
> > >
> > >
> > > --
> > > kvm-riscv mailing list
> > > kvm-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv