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

Message ID	20201015042516.41197-5-takahiro.akashi@linaro.org
State	Accepted
Commit	82e21b391bd315f6fe0e0b79326af8a141e9cca7
Delegated to:	Tom Rini
Headers	show Return-Path: <u-boot-bounces@lists.denx.de> From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: anastasiia_lukianenko@epam.com, oleksandr_andrushchenko@epam.com Cc: u-boot@lists.denx.de, AKASHI Takahiro <takahiro.akashi@linaro.org> Subject: [PATCH 4/4] serial: serial_xen: add DEBUG_UART support Date: Thu, 15 Oct 2020 13:25:16 +0900 Message-Id: <20201015042516.41197-5-takahiro.akashi@linaro.org> In-Reply-To: <20201015042516.41197-1-takahiro.akashi@linaro.org> References: <20201015042516.41197-1-takahiro.akashi@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" <u-boot-bounces@lists.denx.de>
Series	xen: improve console outputs \| expand [0/4] xen: improve console outputs [1/4] serial: serial_xen: print U-Boot banner and others [2/4] arch: arm/xen: add putc() for debugging [3/4] xen: add definitions for console_io [4/4] serial: serial_xen: add DEBUG_UART support

AKASHI Takahiro Oct. 15, 2020, 4:25 a.m. UTC

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(-)

Peng Fan Oct. 15, 2020, 6:50 a.m. UTC | #1

> Subject: [PATCH 4/4] serial: serial_xen: add DEBUG_UART support
> 
> 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.

Please add one more line that it needs XEN debug enabled. without xen debug,
you will not able to see any outputs.

Regards,
Peng.

> +
>  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); #endif }
> +
> +DEBUG_UART_FUNCS
> +
> +#endif
> --
> 2.28.0

Anastasiia Lukianenko Oct. 22, 2020, 9:19 a.m. UTC | #2

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);

> +#endif
> +}
> +
> +DEBUG_UART_FUNCS
> +
> +#endif

Regards,
Anastasiia

AKASHI Takahiro Oct. 22, 2020, 9:53 a.m. UTC | #3

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?

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).)

Thanks,
-Takahiro Akashi


> > +#endif
> > +}
> > +
> > +DEBUG_UART_FUNCS
> > +
> > +#endif
> 
> Regards,
> Anastasiia

Tom Rini Oct. 23, 2020, 12:31 a.m. UTC | #4

On Thu, Oct 15, 2020 at 01:25:16PM +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>

Applied to u-boot/master, thanks!

Anastasiia Lukianenko Oct. 23, 2020, 8:50 a.m. UTC | #5

Hello,

On Thu, 2020-10-22 at 18:53 +0900, takahiro.akashi@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.

> 
> 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).

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

Anastasiia Lukianenko Oct. 23, 2020, 8:53 a.m. UTC | #6

Hi,

On Thu, 2020-10-22 at 18:53 +0900, takahiro.akashi@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?
> 
> 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).)
> 

Sorry, I forgot to write in the last letter the question:
Why can't we simply use HYPERVISOR_console_io(CONSOLEIO_write, 1, (char
*)&ch); for both ARM and Xen?

Regards,
Anastasiia

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

Anastasiia Lukianenko Oct. 23, 2020, 9:22 a.m. UTC | #7

Hello,

On Thu, 2020-10-22 at 20:31 -0400, Tom Rini wrote:
> On Thu, Oct 15, 2020 at 01:25:16PM +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>
> 
> Applied to u-boot/master, thanks!
> 
Sorry, I thought there was still discussion on the series. May I ask
why it was merged?

Regards,
Anastasiia

Tom Rini Oct. 23, 2020, 12:34 p.m. UTC | #8

On Fri, Oct 23, 2020 at 09:22:20AM +0000, Anastasiia Lukianenko wrote:
> Hello,
> 
> On Thu, 2020-10-22 at 20:31 -0400, Tom Rini wrote:
> > On Thu, Oct 15, 2020 at 01:25:16PM +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>
> > 
> > Applied to u-boot/master, thanks!
> > 
> Sorry, I thought there was still discussion on the series. May I ask
> why it was merged?

I just missed it, sorry.  Do you want me too revert the series for now
or should i just expect a small follow-up soon?

Anastasiia Lukianenko Oct. 23, 2020, 1:06 p.m. UTC | #9

On Fri, 2020-10-23 at 08:34 -0400, Tom Rini wrote:
> On Fri, Oct 23, 2020 at 09:22:20AM +0000, Anastasiia Lukianenko
> wrote:
> > Hello,
> > 
> > On Thu, 2020-10-22 at 20:31 -0400, Tom Rini wrote:
> > > On Thu, Oct 15, 2020 at 01:25:16PM +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>
> > > 
> > > Applied to u-boot/master, thanks!
> > > 
> > 
> > Sorry, I thought there was still discussion on the series. May I
> > ask
> > why it was merged?
> 
> I just missed it, sorry.  Do you want me too revert the series for
> now
> or should i just expect a small follow-up soon?
> 
I have no strong opinion here: on one hand there are patches which
won't compile for at least x86 which is obviously not good. On the
other hand we are discussing the rest of the patches as I was not able
to run them on my ARM board, so it either can be that I miss something
and the pacthes are ok or it can also be that the pacthes need to be
re-worked.
So, best was not to have them in the tree yet...

Regards,
Anastasiia

Tom Rini Oct. 23, 2020, 1:18 p.m. UTC | #10

On Fri, Oct 23, 2020 at 01:06:16PM +0000, Anastasiia Lukianenko wrote:
> On Fri, 2020-10-23 at 08:34 -0400, Tom Rini wrote:
> > On Fri, Oct 23, 2020 at 09:22:20AM +0000, Anastasiia Lukianenko
> > wrote:
> > > Hello,
> > > 
> > > On Thu, 2020-10-22 at 20:31 -0400, Tom Rini wrote:
> > > > On Thu, Oct 15, 2020 at 01:25:16PM +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>
> > > > 
> > > > Applied to u-boot/master, thanks!
> > > > 
> > > 
> > > Sorry, I thought there was still discussion on the series. May I
> > > ask
> > > why it was merged?
> > 
> > I just missed it, sorry.  Do you want me too revert the series for
> > now
> > or should i just expect a small follow-up soon?
> > 
> I have no strong opinion here: on one hand there are patches which
> won't compile for at least x86 which is obviously not good. On the
> other hand we are discussing the rest of the patches as I was not able
> to run them on my ARM board, so it either can be that I miss something
> and the pacthes are ok or it can also be that the pacthes need to be
> re-worked.
> So, best was not to have them in the tree yet...

OK, I've reverted the series.  Sorry for the confusion!

AKASHI Takahiro Oct. 26, 2020, 5:58 a.m. UTC | #11

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@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

AKASHI Takahiro Oct. 26, 2020, 6:02 a.m. UTC | #12

On Fri, Oct 23, 2020 at 08:53:52AM +0000, Anastasiia Lukianenko wrote:
> Hi,
> 
> On Thu, 2020-10-22 at 18:53 +0900, takahiro.akashi@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?
> > 
> > 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).)
> > 
> 
> Sorry, I forgot to write in the last letter the question:
> Why can't we simply use HYPERVISOR_console_io(CONSOLEIO_write, 1, (char
> *)&ch); for both ARM and Xen?

Simply because the executed code on Xen side is quite simple.
I assume that simpler it is, more chance of output.

-Takahiro Akashi


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

Oleksandr Andrushchenko Oct. 26, 2020, 6:12 a.m. UTC | #13

Hi,

On 10/26/20 8:02 AM, takahiro.akashi@linaro.org wrote:
> On Fri, Oct 23, 2020 at 08:53:52AM +0000, Anastasiia Lukianenko wrote:
>> Hi,
>>
>> On Thu, 2020-10-22 at 18:53 +0900, takahiro.akashi@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?
>>>
>>> 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).)
>>>
>> Sorry, I forgot to write in the last letter the question:
>> Why can't we simply use HYPERVISOR_console_io(CONSOLEIO_write, 1, (char
>> *)&ch); for both ARM and Xen?
> Simply because the executed code on Xen side is quite simple.
> I assume that simpler it is, more chance of output.

This is true that the simpler the better. And we already have a proper platform

agnostic definition for that:

arch/arm/cpu/armv8/xen/hypercall.S:73:HYPERCALL3(console_io);
arch/arm/include/asm/xen/hypercall.h:16:int HYPERVISOR_console_io(int cmd, int count, char *str);
include/xen/interface/xen.h:44:#define __HYPERVISOR_console_io           18

So, we are not only adding an ARM specific implementation in the patch, but also add

#ifdef's in the code while we can simply avoid that. So, to me, it would make

much more sense if we use what we already have the way Xen has it rather than adding

the code which may require maintenance and make the code harder to read

Thank you,

Oleksandr

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

Oleksandr Andrushchenko Oct. 26, 2020, 6:18 a.m. UTC | #14

Hi,

On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
> 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@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.

The question here is that if x86 is selected it won't compile. Another

question if we tested that with x86: no, we didn't. The reason we tried x86

part was that HYPERVISOR_console_io is a generic definition for all the platforms,

so it was natural to try that as a way to debug things.

>
> 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?

Yes, we did that. You may have noticed that in [1] which I referred earlier

and the problems we faced with that.

> 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?

The code runs on two different platforms with the same result (non-QEMU though).

Thank you,

Oleksandr

>
> Thanks,
> -Takahiro Akashi
>
>
>> Regards,
>> Anastasiia
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>> +DEBUG_UART_FUNCS
>>>>> +
>>>>> +#endif
>>>> Regards,
>>>> Anastasiia
[1] https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html

AKASHI Takahiro Oct. 26, 2020, 6:50 a.m. UTC | #15

On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
> Hi,
> 
> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
> > 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@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.
> 
> The question here is that if x86 is selected it won't compile. Another
> 
> question if we tested that with x86: no, we didn't. The reason we tried x86
> 
> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
> 
> so it was natural to try that as a way to debug things.

Anastasiia said, "No, I just tried different options."
Instead of different options, you tried modified code.

> 
> >
> > 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?
> 
> Yes, we did that. You may have noticed that in [1] which I referred earlier
> and the problems we faced with that.

Okay. Since I started to play with Xen just a few weeks ago,
I actually don't know the past discussions at all.

So the issue you have mentioned has been fixed, hasn't it?
Even if so, you must have submitted your patch in June or later
this year.

Anastasiia said that she had used xen 4.13(+?) to test my code.
So obviously, there will be no chance that you patch be merged
in her test environment.

> 
> > 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?
> 
> The code runs on two different platforms with the same result (non-QEMU though).

Please check on qemu with the latest Xen so that, as I said, we can
determine if the issue is platform or environment (including a difference
of version used) specific or not.
I believe that it is quite easy for you to run U-Boot on qemu.

-Takahiro Akashi

> 
> Thank you,
> 
> Oleksandr
> 
> >
> > Thanks,
> > -Takahiro Akashi
> >
> >
> >> Regards,
> >> Anastasiia
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>
> >>>>> +#endif
> >>>>> +}
> >>>>> +
> >>>>> +DEBUG_UART_FUNCS
> >>>>> +
> >>>>> +#endif
> >>>> Regards,
> >>>> Anastasiia
> [1] https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html

Oleksandr Andrushchenko Oct. 26, 2020, 6:54 a.m. UTC | #16

On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
> On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
>> Hi,
>>
>> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
>>> 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@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.
>> The question here is that if x86 is selected it won't compile. Another
>>
>> question if we tested that with x86: no, we didn't. The reason we tried x86
>>
>> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
>>
>> so it was natural to try that as a way to debug things.
> Anastasiia said, "No, I just tried different options."
> Instead of different options, you tried modified code.

That was to debug why the original code didn't work, I see nothing wrong

in that she tried helping you to figure out why...

>
>>> 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?
>> Yes, we did that. You may have noticed that in [1] which I referred earlier
>> and the problems we faced with that.
> Okay. Since I started to play with Xen just a few weeks ago,
> I actually don't know the past discussions at all.
>
> So the issue you have mentioned has been fixed, hasn't it?
> Even if so, you must have submitted your patch in June or later
> this year.
>
> Anastasiia said that she had used xen 4.13(+?) to test my code.
> So obviously, there will be no chance that you patch be merged
> in her test environment.
>
>>> 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?
>> The code runs on two different platforms with the same result (non-QEMU though).
> Please check on qemu with the latest Xen so that, as I said, we can
> determine if the issue is platform or environment (including a difference
> of version used) specific or not.
> I believe that it is quite easy for you to run U-Boot on qemu.
>
> -Takahiro Akashi
>
>> Thank you,
>>
>> Oleksandr
>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>> Regards,
>>>> Anastasiia
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>>> +#endif
>>>>>>> +}
>>>>>>> +
>>>>>>> +DEBUG_UART_FUNCS
>>>>>>> +
>>>>>>> +#endif
>>>>>> Regards,
>>>>>> Anastasiia
>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

AKASHI Takahiro Oct. 26, 2020, 7:10 a.m. UTC | #17

On Mon, Oct 26, 2020 at 06:54:29AM +0000, Oleksandr Andrushchenko wrote:
> 
> On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
> > On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
> >> Hi,
> >>
> >> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
> >>> 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@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.
> >> The question here is that if x86 is selected it won't compile. Another
> >>
> >> question if we tested that with x86: no, we didn't. The reason we tried x86
> >>
> >> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
> >>
> >> so it was natural to try that as a way to debug things.
> > Anastasiia said, "No, I just tried different options."
> > Instead of different options, you tried modified code.
> 
> That was to debug why the original code didn't work, I see nothing wrong
> 
> in that she tried helping you to figure out why...

I really appreciate your assistance here.

But without knowing the detailed environment on your side, it may sometimes
be difficult to find out the root cause.

> >
> >>> 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?
> >> Yes, we did that. You may have noticed that in [1] which I referred earlier
> >> and the problems we faced with that.
> > Okay. Since I started to play with Xen just a few weeks ago,
> > I actually don't know the past discussions at all.
> >
> > So the issue you have mentioned has been fixed, hasn't it?

Please confirm this.

> > Even if so, you must have submitted your patch in June or later
> > this year.
> >
> > Anastasiia said that she had used xen 4.13(+?) to test my code.
> > So obviously, there will be no chance that you patch be merged
> > in her test environment.
> >
> >>> 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?
> >> The code runs on two different platforms with the same result (non-QEMU though).
> > Please check on qemu with the latest Xen so that, as I said, we can
> > determine if the issue is platform or environment (including a difference
> > of version used) specific or not.
> > I believe that it is quite easy for you to run U-Boot on qemu.

Please try this first. I believe that it is the first step to take.

Since I don't have any real hardwrare at this moment,
it will be difficult for me to dig into the issue
unless it can be reproduced on qemu.

Thanks,
-Takahiro Akashi


> >
> > -Takahiro Akashi
> >
> >> Thank you,
> >>
> >> Oleksandr
> >>
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>
> >>>> Regards,
> >>>> Anastasiia
> >>>>> Thanks,
> >>>>> -Takahiro Akashi
> >>>>>
> >>>>>
> >>>>>>> +#endif
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +DEBUG_UART_FUNCS
> >>>>>>> +
> >>>>>>> +#endif
> >>>>>> Regards,
> >>>>>> Anastasiia
> >> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

Oleksandr Andrushchenko Oct. 26, 2020, 7:16 a.m. UTC | #18

Hi,

On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
> On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
>> Hi,
>>
>> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
>>> 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@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.
>> The question here is that if x86 is selected it won't compile. Another
>>
>> question if we tested that with x86: no, we didn't. The reason we tried x86
>>
>> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
>>
>> so it was natural to try that as a way to debug things.
> Anastasiia said, "No, I just tried different options."
> Instead of different options, you tried modified code.
>
>>> 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?
>> Yes, we did that. You may have noticed that in [1] which I referred earlier
>> and the problems we faced with that.
> Okay. Since I started to play with Xen just a few weeks ago,
> I actually don't know the past discussions at all.

So, this is why I provided you with a link to previous discussion we had on

Xen development list: that was with respect to console_io and D-cache maintenance.

That was actually the reason we didn't put debug console as a part of the initial

submission.

>
> So the issue you have mentioned has been fixed, hasn't it?

No, why? It is all defined in Xen ARM's ABI and u-boot must take care of it on its end

as it violates the ABI before the MMU is on.

> Even if so, you must have submitted your patch in June or later
> this year.
Hm...
>
> Anastasiia said that she had used xen 4.13(+?) to test my code.
> So obviously, there will be no chance that you patch be merged
> in her test environment.

Ok, once again. This is about the ABI which is stable. Do you think more recent Xen

broke something?

>
>>> 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?
>> The code runs on two different platforms with the same result (non-QEMU though).
> Please check on qemu with the latest Xen so that, as I said, we can
> determine if the issue is platform or environment (including a difference
> of version used) specific or not.
> I believe that it is quite easy for you to run U-Boot on qemu.

Well, I have probably missed something, but you neither mentioned you are running

under QEMU (my apologies if this is not true) nor you have provided the exact configuration

of your setup, e.g. QEMU's command line, what is used as a root fs etc. so one interested

in the topic can actually reproduce your setup.

Thank you,

Oleksandr

>
> -Takahiro Akashi
>
>> Thank you,
>>
>> Oleksandr
>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>> Regards,
>>>> Anastasiia
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>>> +#endif
>>>>>>> +}
>>>>>>> +
>>>>>>> +DEBUG_UART_FUNCS
>>>>>>> +
>>>>>>> +#endif
>>>>>> Regards,
>>>>>> Anastasiia
>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

Oleksandr Andrushchenko Oct. 26, 2020, 7:30 a.m. UTC | #19

On 10/26/20 9:10 AM, takahiro.akashi@linaro.org wrote:
> On Mon, Oct 26, 2020 at 06:54:29AM +0000, Oleksandr Andrushchenko wrote:
>> On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
>>> On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
>>>> Hi,
>>>>
>>>> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
>>>>> 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@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.
>>>> The question here is that if x86 is selected it won't compile. Another
>>>>
>>>> question if we tested that with x86: no, we didn't. The reason we tried x86
>>>>
>>>> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
>>>>
>>>> so it was natural to try that as a way to debug things.
>>> Anastasiia said, "No, I just tried different options."
>>> Instead of different options, you tried modified code.
>> That was to debug why the original code didn't work, I see nothing wrong
>>
>> in that she tried helping you to figure out why...
> I really appreciate your assistance here.

We are really interested in what you do as we didn't have enough cycles to do the same

at the time of the initial submission. And we can help testing that on 2 different

HW platforms: Renesas and iMX8. Also, Xen devel community and us will be glad to

help you with this

>
> But without knowing the detailed environment on your side, it may sometimes
> be difficult to find out the root cause.

I believe this part must be platform agnostic, e.g. it should work the same way

on any platform

>
>>>>> 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?
>>>> Yes, we did that. You may have noticed that in [1] which I referred earlier
>>>> and the problems we faced with that.
>>> Okay. Since I started to play with Xen just a few weeks ago,
>>> I actually don't know the past discussions at all.
>>>
>>> So the issue you have mentioned has been fixed, hasn't it?
> Please confirm this.

Xen ARM ABI is stable for a long time now, so it is confirmed as not related

to possible code changes, but ABI violation on u-boot side before the MMU is on

(this is basically where we need debug console most of the time).

>
>>> Even if so, you must have submitted your patch in June or later
>>> this year.
>>>
>>> Anastasiia said that she had used xen 4.13(+?) to test my code.
>>> So obviously, there will be no chance that you patch be merged
>>> in her test environment.
>>>
>>>>> 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?
>>>> The code runs on two different platforms with the same result (non-QEMU though).
>>> Please check on qemu with the latest Xen so that, as I said, we can
>>> determine if the issue is platform or environment (including a difference
>>> of version used) specific or not.
>>> I believe that it is quite easy for you to run U-Boot on qemu.
> Please try this first. I believe that it is the first step to take.
Please provide the exact environment you use, so we can have the same on our side
>
> Since I don't have any real hardwrare at this moment,
> it will be difficult for me to dig into the issue
> unless it can be reproduced on qemu.

Understand you and as I said above we can help testing this on real HW

Thank you,

Oleksandr

>
> Thanks,
> -Takahiro Akashi
>
>
>>> -Takahiro Akashi
>>>
>>>> Thank you,
>>>>
>>>> Oleksandr
>>>>
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>> Regards,
>>>>>> Anastasiia
>>>>>>> Thanks,
>>>>>>> -Takahiro Akashi
>>>>>>>
>>>>>>>
>>>>>>>>> +#endif
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +DEBUG_UART_FUNCS
>>>>>>>>> +
>>>>>>>>> +#endif
>>>>>>>> Regards,
>>>>>>>> Anastasiia
>>>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

AKASHI Takahiro Oct. 26, 2020, 8:03 a.m. UTC | #20

Oleksandr,

It seems that we now stand on the same page.

On Mon, Oct 26, 2020 at 07:30:06AM +0000, Oleksandr Andrushchenko wrote:
> 
> On 10/26/20 9:10 AM, takahiro.akashi@linaro.org wrote:
> > On Mon, Oct 26, 2020 at 06:54:29AM +0000, Oleksandr Andrushchenko wrote:
> >> On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
> >>> On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
> >>>> Hi,
> >>>>
> >>>> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
> >>>>> 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@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.
> >>>> The question here is that if x86 is selected it won't compile. Another
> >>>>
> >>>> question if we tested that with x86: no, we didn't. The reason we tried x86
> >>>>
> >>>> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
> >>>>
> >>>> so it was natural to try that as a way to debug things.
> >>> Anastasiia said, "No, I just tried different options."
> >>> Instead of different options, you tried modified code.
> >> That was to debug why the original code didn't work, I see nothing wrong
> >>
> >> in that she tried helping you to figure out why...
> > I really appreciate your assistance here.
> 
> We are really interested in what you do as we didn't have enough cycles to do the same
> at the time of the initial submission. And we can help testing that on 2 different
> HW platforms: Renesas and iMX8. Also, Xen devel community and us will be glad to
> help you with this

Thank you again.

> >
> > But without knowing the detailed environment on your side, it may sometimes
> > be difficult to find out the root cause.
> 
> I believe this part must be platform agnostic, e.g. it should work the same way
> 
> on any platform

Of course, agree.

> >
> >>>>> 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?
> >>>> Yes, we did that. You may have noticed that in [1] which I referred earlier
> >>>> and the problems we faced with that.
> >>> Okay. Since I started to play with Xen just a few weeks ago,
> >>> I actually don't know the past discussions at all.
> >>>
> >>> So the issue you have mentioned has been fixed, hasn't it?
> > Please confirm this.
> 
> Xen ARM ABI is stable for a long time now, so it is confirmed as not related
> to possible code changes, but ABI violation on u-boot side before the MMU is on
> (this is basically where we need debug console most of the time).

Still I'm a bit confused.

After all, HYPERBVISOR_console_io doesn't work yet on arm/arm64,
at least, in an early boot stage of U-Boot.

Is this the right understanding about current HYPERVISOR_console_io support?

> >
> >>> Even if so, you must have submitted your patch in June or later
> >>> this year.
> >>>
> >>> Anastasiia said that she had used xen 4.13(+?) to test my code.
> >>> So obviously, there will be no chance that you patch be merged
> >>> in her test environment.
> >>>
> >>>>> 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?
> >>>> The code runs on two different platforms with the same result (non-QEMU though).
> >>> Please check on qemu with the latest Xen so that, as I said, we can
> >>> determine if the issue is platform or environment (including a difference
> >>> of version used) specific or not.
> >>> I believe that it is quite easy for you to run U-Boot on qemu.
> > Please try this first. I believe that it is the first step to take.
> Please provide the exact environment you use, so we can have the same on our side

host: debian 20.04
qemu: debian's qemu (4.2.1) or my own build (of 5.1.0)

qemu cmd params:
        -serial mon:stdio -nographic -boot menu=on
        -machine virt,gic-version=3,virtualization=on,secure=on
        -cpu cortex-a57 -smp 2 -m 4G -d unimp
        ... (misc virtio disks)
        -rtc base=utc
        -bios /path/to/rom.bin
        (I use tf-a + u-boot to boot xen+debian.)

xen: upstream 4.15+ (25849c8b16f2 "xen/rpi4: implement watchdog-based reset")
     with default configuration (maybe adding "#undef NDEBUG")
xen boot params:
        sync_console dom0_mem=2G loglvl=all guest_loglvl=all

dom0: debian testing (as of Oct 5th?) + my own built xen/xentools (see above)

u-boot: upstream v2020.10 (or pre-v2021.01-rc1) + my patch
     with xenguest_arm64_defconfig + CONFIG_DEBUG_UART
          (+ misc configurations)
domU config:
        kernel = "/boot/u-boot.bin"
        memory = 128
        vcpus = 1

Please let me know if you need more information.

Thanks,
-Takahiro Akashi

> >
> > Since I don't have any real hardwrare at this moment,
> > it will be difficult for me to dig into the issue
> > unless it can be reproduced on qemu.
> 
> Understand you and as I said above we can help testing this on real HW
> 
> Thank you,
> 
> Oleksandr
> 
> >
> > Thanks,
> > -Takahiro Akashi
> >
> >
> >>> -Takahiro Akashi
> >>>
> >>>> Thank you,
> >>>>
> >>>> Oleksandr
> >>>>
> >>>>> Thanks,
> >>>>> -Takahiro Akashi
> >>>>>
> >>>>>
> >>>>>> Regards,
> >>>>>> Anastasiia
> >>>>>>> Thanks,
> >>>>>>> -Takahiro Akashi
> >>>>>>>
> >>>>>>>
> >>>>>>>>> +#endif
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +DEBUG_UART_FUNCS
> >>>>>>>>> +
> >>>>>>>>> +#endif
> >>>>>>>> Regards,
> >>>>>>>> Anastasiia
> >>>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

Oleksandr Andrushchenko Oct. 26, 2020, 8:19 a.m. UTC | #21

On 10/26/20 10:03 AM, takahiro.akashi@linaro.org wrote:
> Oleksandr,
>
> It seems that we now stand on the same page.
Great, hope all together we can make it happen
>
> On Mon, Oct 26, 2020 at 07:30:06AM +0000, Oleksandr Andrushchenko wrote:
>> On 10/26/20 9:10 AM, takahiro.akashi@linaro.org wrote:
>>> On Mon, Oct 26, 2020 at 06:54:29AM +0000, Oleksandr Andrushchenko wrote:
>>>> On 10/26/20 8:50 AM, takahiro.akashi@linaro.org wrote:
>>>>> On Mon, Oct 26, 2020 at 06:18:08AM +0000, Oleksandr Andrushchenko wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/26/20 7:58 AM, takahiro.akashi@linaro.org wrote:
>>>>>>> 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@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.
>>>>>> The question here is that if x86 is selected it won't compile. Another
>>>>>>
>>>>>> question if we tested that with x86: no, we didn't. The reason we tried x86
>>>>>>
>>>>>> part was that HYPERVISOR_console_io is a generic definition for all the platforms,
>>>>>>
>>>>>> so it was natural to try that as a way to debug things.
>>>>> Anastasiia said, "No, I just tried different options."
>>>>> Instead of different options, you tried modified code.
>>>> That was to debug why the original code didn't work, I see nothing wrong
>>>>
>>>> in that she tried helping you to figure out why...
>>> I really appreciate your assistance here.
>> We are really interested in what you do as we didn't have enough cycles to do the same
>> at the time of the initial submission. And we can help testing that on 2 different
>> HW platforms: Renesas and iMX8. Also, Xen devel community and us will be glad to
>> help you with this
> Thank you again.
>
>>> But without knowing the detailed environment on your side, it may sometimes
>>> be difficult to find out the root cause.
>> I believe this part must be platform agnostic, e.g. it should work the same way
>>
>> on any platform
> Of course, agree.
>
>>>>>>> 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?
>>>>>> Yes, we did that. You may have noticed that in [1] which I referred earlier
>>>>>> and the problems we faced with that.
>>>>> Okay. Since I started to play with Xen just a few weeks ago,
>>>>> I actually don't know the past discussions at all.
>>>>>
>>>>> So the issue you have mentioned has been fixed, hasn't it?
>>> Please confirm this.
>> Xen ARM ABI is stable for a long time now, so it is confirmed as not related
>> to possible code changes, but ABI violation on u-boot side before the MMU is on
>> (this is basically where we need debug console most of the time).
> Still I'm a bit confused.
>
> After all, HYPERBVISOR_console_io doesn't work yet on arm/arm64,
> at least, in an early boot stage of U-Boot.
>
> Is this the right understanding about current HYPERVISOR_console_io support?

I do suggest you read [1] for better understanding of the issue we faced with

early debug console. In our setup we, without having debug UART implementation,

use the two following for debugging:

staticvoidxen_serial_putc(constcharc)
{
     invalidate_dcache_range((unsignedlong)&c,
                 (unsignedlong)&c + 1);
     (void)HYPERVISOR_console_io(CONSOLEIO_write, 1, (char*)&c);
}
staticvoidxen_serial_puts(constchar*str)
{
intlen = strlen(str);
     invalidate_dcache_range((unsignedlong)str,
                 (unsignedlong)str + len);
     (void)HYPERVISOR_console_io(CONSOLEIO_write, len, (char*)str);
}
Please not those invalidate_dcache_range calls. With the above we can see
debug prints way before the DM based serial driver is initialized and MMU is set up
(this is required because of D-cache on ARM).
>
>>>>> Even if so, you must have submitted your patch in June or later
>>>>> this year.
>>>>>
>>>>> Anastasiia said that she had used xen 4.13(+?) to test my code.
>>>>> So obviously, there will be no chance that you patch be merged
>>>>> in her test environment.
>>>>>
>>>>>>> 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?
>>>>>> The code runs on two different platforms with the same result (non-QEMU though).
>>>>> Please check on qemu with the latest Xen so that, as I said, we can
>>>>> determine if the issue is platform or environment (including a difference
>>>>> of version used) specific or not.
>>>>> I believe that it is quite easy for you to run U-Boot on qemu.
>>> Please try this first. I believe that it is the first step to take.
>> Please provide the exact environment you use, so we can have the same on our side
> host: debian 20.04
> qemu: debian's qemu (4.2.1) or my own build (of 5.1.0)
>
> qemu cmd params:
>          -serial mon:stdio -nographic -boot menu=on
>          -machine virt,gic-version=3,virtualization=on,secure=on
>          -cpu cortex-a57 -smp 2 -m 4G -d unimp
>          ... (misc virtio disks)
>          -rtc base=utc
>          -bios /path/to/rom.bin
>          (I use tf-a + u-boot to boot xen+debian.)
>
> xen: upstream 4.15+ (25849c8b16f2 "xen/rpi4: implement watchdog-based reset")
>       with default configuration (maybe adding "#undef NDEBUG")
> xen boot params:
>          sync_console dom0_mem=2G loglvl=all guest_loglvl=all
>
> dom0: debian testing (as of Oct 5th?) + my own built xen/xentools (see above)
>
> u-boot: upstream v2020.10 (or pre-v2021.01-rc1) + my patch
>       with xenguest_arm64_defconfig + CONFIG_DEBUG_UART
>            (+ misc configurations)
> domU config:
>          kernel = "/boot/u-boot.bin"
>          memory = 128
>          vcpus = 1
>
> Please let me know if you need more information.
Thank you
>
> Thanks,
> -Takahiro Akashi

Thank you,

Oleksandr

>
>>> Since I don't have any real hardwrare at this moment,
>>> it will be difficult for me to dig into the issue
>>> unless it can be reproduced on qemu.
>> Understand you and as I said above we can help testing this on real HW
>>
>> Thank you,
>>
>> Oleksandr
>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>>> -Takahiro Akashi
>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Oleksandr
>>>>>>
>>>>>>> Thanks,
>>>>>>> -Takahiro Akashi
>>>>>>>
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Anastasiia
>>>>>>>>> Thanks,
>>>>>>>>> -Takahiro Akashi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> +#endif
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +DEBUG_UART_FUNCS
>>>>>>>>>>> +
>>>>>>>>>>> +#endif
>>>>>>>>>> Regards,
>>>>>>>>>> Anastasiia
>>>>>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-06/msg00737.html__;!!GF_29dbcQIUBPA!lbY6WN0YDxFiqUvVTZI8ZkwzoiY08y_oHciOZ7lrUxxpdo-aX37PHDwcSwc3Mb_7uRivJJpP0A$ [lists[.]xenproject[.]org]

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

Commit Message

Comments

Patch