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

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

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 at 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 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.
> 
> 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]