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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 10/26/20 10:03 AM, takahiro.akashi at 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 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?

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]