[PATCH 4/4] serial: serial_xen: add DEBUG_UART support

[takahiro.akashi at linaro.org <takahiro.akashi@linaro.org>]

On Fri, Oct 23, 2020 at 08:50:56AM +0000, Anastasiia Lukianenko wrote:
> Hello,
> 
> On Thu, 2020-10-22 at 18:53 +0900, takahiro.akashi at linaro.org wrote:
> > On Thu, Oct 22, 2020 at 09:19:41AM +0000, Anastasiia Lukianenko
> > wrote:
> > > Hi,
> > > 
> > > On Thu, 2020-10-15 at 13:25 +0900, AKASHI Takahiro wrote:
> > > > By using a hypervisor call, we can implement DEBUG_UART on xen.
> > > > This will allow us to see messages even earlier than
> > > > serial_init().
> > > > 
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  drivers/serial/Kconfig      | 14 +++++++++++---
> > > >  drivers/serial/serial_xen.c | 20 ++++++++++++++++++++
> > > >  2 files changed, 31 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > > > index e344677f91f6..536cf0641773 100644
> > > > --- a/drivers/serial/Kconfig
> > > > +++ b/drivers/serial/Kconfig
> > > > @@ -401,11 +401,19 @@ config DEBUG_UART_MTK
> > > >  	  driver will be available until the real driver model serial
> > > > is
> > > >  	  running.
> > > >  
> > > > +config DEBUG_UART_XEN
> > > > +	bool "XEN Hypervisor Console"
> > > > +	depends on XEN_SERIAL
> > > > +	help
> > > > +	  Select this to enable a debug UART using the serial_xen
> > > > driver. You
> > > > +	  will not have to provide any parameters to make this work.
> > > > The driver
> > > > +          will be available until the real driver-model serial
> > > > is
> > > > running.
> > > > +
> > > >  endchoice
> > > >  
> > > >  config DEBUG_UART_BASE
> > > >  	hex "Base address of UART"
> > > > -	depends on DEBUG_UART
> > > > +	depends on DEBUG_UART && !DEBUG_UART_XEN
> > > >  	default 0 if DEBUG_UART_SANDBOX
> > > >  	help
> > > >  	  This is the base address of your UART for memory-mapped
> > > > UARTs.
> > > > @@ -415,7 +423,7 @@ config DEBUG_UART_BASE
> > > >  
> > > >  config DEBUG_UART_CLOCK
> > > >  	int "UART input clock"
> > > > -	depends on DEBUG_UART
> > > > +	depends on DEBUG_UART && !DEBUG_UART_XEN
> > > >  	default 0 if DEBUG_UART_SANDBOX
> > > >  	help
> > > >  	  The UART input clock determines the speed of the internal
> > > > UART
> > > > @@ -427,7 +435,7 @@ config DEBUG_UART_CLOCK
> > > >  
> > > >  config DEBUG_UART_SHIFT
> > > >  	int "UART register shift"
> > > > -	depends on DEBUG_UART
> > > > +	depends on DEBUG_UART && !DEBUG_UART_XEN
> > > >  	default 0 if DEBUG_UART
> > > >  	help
> > > >  	  Some UARTs (notably ns16550) support different register
> > > > layouts
> > > > diff --git a/drivers/serial/serial_xen.c
> > > > b/drivers/serial/serial_xen.c
> > > > index ed191829f059..34c90ece40fc 100644
> > > > --- a/drivers/serial/serial_xen.c
> > > > +++ b/drivers/serial/serial_xen.c
> > > > @@ -5,6 +5,7 @@
> > > >   */
> > > >  #include <common.h>
> > > >  #include <cpu_func.h>
> > > > +#include <debug_uart.h>
> > > >  #include <dm.h>
> > > >  #include <serial.h>
> > > >  #include <watchdog.h>
> > > > @@ -15,11 +16,14 @@
> > > >  #include <xen/events.h>
> > > >  
> > > >  #include <xen/interface/sched.h>
> > > > +#include <xen/interface/xen.h>
> > > >  #include <xen/interface/hvm/hvm_op.h>
> > > >  #include <xen/interface/hvm/params.h>
> > > >  #include <xen/interface/io/console.h>
> > > >  #include <xen/interface/io/ring.h>
> > > >  
> > > > +#include <asm/xen/hypercall.h>
> > > > +
> > > >  DECLARE_GLOBAL_DATA_PTR;
> > > >  
> > > >  u32 console_evtchn;
> > > > @@ -178,3 +182,19 @@ U_BOOT_DRIVER(serial_xen) = {
> > > >  	.flags			= DM_FLAG_PRE_RELOC,
> > > >  };
> > > >  
> > > > +#if defined(CONFIG_DEBUG_UART_XEN)
> > > > +static inline void _debug_uart_init(void) {}
> > > > +
> > > > +static inline void _debug_uart_putc(int c)
> > > > +{
> > > > +#if CONFIG_IS_ENABLED(ARM)
> > > > +	xen_debug_putc(c);
> > > > +#else
> > > > +	/* the type cast should work on LE only */
> > > > +	HYPERVISOR_console_io(CONSOLEIO_write, 1, (char *)&ch);
> > > 
> > > An error occurs during compilation:
> > > drivers/serial/serial_xen.c: error: ?ch? undeclared (first use in
> > > this
> > > function); did you mean ?c??
> > >         HYPERVISOR_console_io(CONSOLEIO_write, 1, (char *)&ch);
> > 
> > Ah, yes. You're now using x86, right?
> 
> No, I just tried different options and accidentally discovered this
> error.

No?
My code is protected with "if CONFIG_IS_ENABLED(ARM)", and so
you have no chance of building "else" clause unless you use x86.

Anyway,

> > 
> > So what happens if you have made the fix above?
> > Does it work in your environment?
> > (I have confirmed that HYPERVISOR_console_io version works on
> > arm(64).)
> 
> Unfortunately, nothing happened (there are no logs mentioned earlier).

1. Have you ever executed HYPERVISOR_console_io on your platform before?
2. If possible, please try to my code on qemu, as my patch intended, so that
   we can determine if your issue is generic or specific on your environment?

Thanks,
-Takahiro Akashi


> Regards,
> Anastasiia
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> > > > +#endif
> > > > +}
> > > > +
> > > > +DEBUG_UART_FUNCS
> > > > +
> > > > +#endif
> > > 
> > > Regards,
> > > Anastasiia