Re: [PATCH v1 14/14] xen/riscv: add basic UART support

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/15/25 6:03 PM, Jan Beulich wrote:

> On 08.04.2025 17:57, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -4,11 +4,16 @@
> >  #include <xen/bug.h>
> >  #include <xen/bootfdt.h>
> >  #include <xen/compile.h>
> > +#include <xen/console.h>
> >  #include <xen/device_tree.h>
> >  #include <xen/init.h>
> >  #include <xen/irq.h>
> > +#include <xen/keyhandler.h>
> >       
> Why's this one needed?
>     
It isn't needed anymore, just missed to drop.
I thought that it would be needed to test UART working fine by checking if Ctrl+AAA is working.

> >  #include <xen/mm.h>
> > +#include <xen/percpu.h>
> > +#include <xen/serial.h>
> >  #include <xen/shutdown.h>
> > +#include <xen/timer.h>
> >  #include <xen/vmap.h>
> >  #include <xen/xvmalloc.h>
> >  
> > @@ -73,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
> >  
> >      remove_identity_mapping();
> >  
> > +    percpu_init_areas();
> >       
> I'll trust you that it's needed now, but the addition looks unrelated here,
> and also isn't mentioned as intentional in the description.
>     
I had  this patch when I used polling mode for UART, for this case percpu is used to receive
serial port info:
  struct serial_port *port = this_cpu(poll_port);

So percpu isn't really needed at the current development state. I'll drop this change or as an option
move to separate patch.

> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -3,7 +3,6 @@ config GENERIC_UART_INIT
> >  
> >  config HAS_NS16550
> >  	bool "NS16550 UART driver" if ARM
> > -	default n if RISCV
> >  	default y
> >       
> Just to double-check: Unlike Arm you don't want this to be user-(un)selectable
> on RISC-V?
>     
Thanks for noticing that. I want to have this selectable by user. I will add RISC-V here.

~ Oleksii