On 10/26/20 10:03 AM, takahiro.aka...@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.aka...@linaro.org wrote: >>> On Mon, Oct 26, 2020 at 06:54:29AM +0000, Oleksandr Andrushchenko wrote: >>>> On 10/26/20 8:50 AM, takahiro.aka...@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.aka...@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.aka...@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.aka...@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]