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

Oleksandr Andrushchenko
2020-10-26T06:54:29Z

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

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