[PATCH v1 14/14] xen/riscv: add basic UART support

Oleksii Kurochko posted 14 patches 8 months, 1 week ago
There is a newer version of this series
[PATCH v1 14/14] xen/riscv: add basic UART support
Posted by Oleksii Kurochko 8 months, 1 week ago
Update Kconfig to select GENERIC_UART_INIT for basic UART init ( find a dt node
and call device specific device_init() ).

Drop `default n if RISCV` statement for config HAS_NS16550 as now ns16550 is
ready to be compiled and used by RISC-V.

Initialize a minimal amount of stuff to have UART and Xen console:
 - Initialize uart by calling uart_init().
 - Initialize Xen console by calling console_init_{pre,post}irq().
 - Initialize timer and its internal lists which are used by
   init_timer() which is called by ns16550_init_postirq(); otherwise
   "Unhandled exception: Store/AMO Page Fault" occurs.
 - Enable local interrupt to recieve an input from UART

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Kconfig   |  1 +
 xen/arch/riscv/setup.c   | 16 ++++++++++++++++
 xen/drivers/char/Kconfig |  1 -
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 27086cca9c..f5ba7a5a78 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -2,6 +2,7 @@ config RISCV
 	def_bool y
 	select FUNCTION_ALIGNMENT_16B
 	select GENERIC_BUG_FRAME
+	select GENERIC_UART_INIT
 	select HAS_DEVICE_TREE
 	select HAS_PMAP
 	select HAS_UBSAN
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9765bcbb08..b5fd660a4b 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -4,11 +4,16 @@
 #include <xen/bug.h>
 #include <xen/bootfdt.h>
 #include <xen/compile.h>
+#include <xen/console.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
 #include <xen/irq.h>
+#include <xen/keyhandler.h>
 #include <xen/mm.h>
+#include <xen/percpu.h>
+#include <xen/serial.h>
 #include <xen/shutdown.h>
+#include <xen/timer.h>
 #include <xen/vmap.h>
 #include <xen/xvmalloc.h>
 
@@ -73,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     remove_identity_mapping();
 
+    percpu_init_areas();
+
     smp_clear_cpu_maps();
 
     set_processor_id(0);
@@ -136,8 +143,17 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     intc_preinit();
 
+    uart_init();
+    console_init_preirq();
+
     intc_init();
 
+    timer_init();
+
+    local_irq_enable();
+
+    console_init_postirq();
+
     printk("All set up\n");
 
     machine_halt();
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e6e12bb413..01fa31fb2b 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -3,7 +3,6 @@ config GENERIC_UART_INIT
 
 config HAS_NS16550
 	bool "NS16550 UART driver" if ARM
-	default n if RISCV
 	default y
 	help
 	  This selects the 16550-series UART support. For most systems, say Y.
-- 
2.49.0
Re: [PATCH v1 14/14] xen/riscv: add basic UART support
Posted by Jan Beulich 8 months ago
On 08.04.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -4,11 +4,16 @@
>  #include <xen/bug.h>
>  #include <xen/bootfdt.h>
>  #include <xen/compile.h>
> +#include <xen/console.h>
>  #include <xen/device_tree.h>
>  #include <xen/init.h>
>  #include <xen/irq.h>
> +#include <xen/keyhandler.h>

Why's this one needed?

>  #include <xen/mm.h>
> +#include <xen/percpu.h>
> +#include <xen/serial.h>
>  #include <xen/shutdown.h>
> +#include <xen/timer.h>
>  #include <xen/vmap.h>
>  #include <xen/xvmalloc.h>
>  
> @@ -73,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      remove_identity_mapping();
>  
> +    percpu_init_areas();

I'll trust you that it's needed now, but the addition looks unrelated here,
and also isn't mentioned as intentional in the description.

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -3,7 +3,6 @@ config GENERIC_UART_INIT
>  
>  config HAS_NS16550
>  	bool "NS16550 UART driver" if ARM
> -	default n if RISCV
>  	default y

Just to double-check: Unlike Arm you don't want this to be user-(un)selectable
on RISC-V?

Jan
Re: [PATCH v1 14/14] xen/riscv: add basic UART support
Posted by Oleksii Kurochko 8 months ago
On 4/15/25 6:03 PM, Jan Beulich wrote:
> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/setup.c
>> +++ b/xen/arch/riscv/setup.c
>> @@ -4,11 +4,16 @@
>>   #include <xen/bug.h>
>>   #include <xen/bootfdt.h>
>>   #include <xen/compile.h>
>> +#include <xen/console.h>
>>   #include <xen/device_tree.h>
>>   #include <xen/init.h>
>>   #include <xen/irq.h>
>> +#include <xen/keyhandler.h>
> Why's this one needed?

It isn't needed anymore, just missed to drop.
I thought that it would be needed to test UART working fine by checking if Ctrl+AAA is working.

>
>>   #include <xen/mm.h>
>> +#include <xen/percpu.h>
>> +#include <xen/serial.h>
>>   #include <xen/shutdown.h>
>> +#include <xen/timer.h>
>>   #include <xen/vmap.h>
>>   #include <xen/xvmalloc.h>
>>   
>> @@ -73,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>   
>>       remove_identity_mapping();
>>   
>> +    percpu_init_areas();
> I'll trust you that it's needed now, but the addition looks unrelated here,
> and also isn't mentioned as intentional in the description.

I had  this patch when I used polling mode for UART, for this case percpu is used to receive
serial port info:
   struct serial_port *port = this_cpu(poll_port);

So percpu isn't really needed at the current development state. I'll drop this change or as an option
move to separate patch.

>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -3,7 +3,6 @@ config GENERIC_UART_INIT
>>   
>>   config HAS_NS16550
>>   	bool "NS16550 UART driver" if ARM
>> -	default n if RISCV
>>   	default y
> Just to double-check: Unlike Arm you don't want this to be user-(un)selectable
> on RISC-V?

Thanks for noticing that. I want to have this selectable by user. I will add RISC-V here.

~ Oleksii
Re: [PATCH v1 14/14] xen/riscv: add basic UART support
Posted by Jan Beulich 8 months ago
On 17.04.2025 12:31, Oleksii Kurochko wrote:
> On 4/15/25 6:03 PM, Jan Beulich wrote:
>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>> --- a/xen/drivers/char/Kconfig
>>> +++ b/xen/drivers/char/Kconfig
>>> @@ -3,7 +3,6 @@ config GENERIC_UART_INIT
>>>   
>>>   config HAS_NS16550
>>>   	bool "NS16550 UART driver" if ARM
>>> -	default n if RISCV
>>>   	default y
>> Just to double-check: Unlike Arm you don't want this to be user-(un)selectable
>> on RISC-V?
> 
> Thanks for noticing that. I want to have this selectable by user. I will add RISC-V here.

At which point we may want to consider whether the condition on the prompt
wouldn't better become "!X86".

Jan