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