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

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

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