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

Oleksandr Andrushchenko Oleksandr_Andrushchenko at epam.com
Mon Oct 26 08:30:06 CET 2020 

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

More information about the U-Boot mailing list