[v1] riscv: introduce basic UART support and interrupts for hypervisor mode

[PATCH v1 13/14] xen/riscv: initialize interrupt controller

Posted by Oleksii Kurochko 8 months, 1 week ago

Call intc_init() to do basic initialization steps for APLIC and IMISC.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/setup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index a3189697da..9765bcbb08 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     intc_preinit();
 
+    intc_init();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.49.0

Re: [PATCH v1 13/14] xen/riscv: initialize interrupt controller

Posted by Jan Beulich 8 months ago

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> Call intc_init() to do basic initialization steps for APLIC and IMISC.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
yet ...

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      intc_preinit();
>  
> +    intc_init();
> +
>      printk("All set up\n");
>  
>      machine_halt();

... this being everything here I wonder if this can't be folded with the
patch where the function is introduced.

Jan

Re: [PATCH v1 13/14] xen/riscv: initialize interrupt controller

Posted by Oleksii Kurochko 8 months ago

On 4/15/25 5:59 PM, Jan Beulich wrote:
> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>> Call intc_init() to do basic initialization steps for APLIC and IMISC.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich<jbeulich@suse.com>
> yet ...
>
>> --- a/xen/arch/riscv/setup.c
>> +++ b/xen/arch/riscv/setup.c
>> @@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>   
>>       intc_preinit();
>>   
>> +    intc_init();
>> +
>>       printk("All set up\n");
>>   
>>       machine_halt();
> ... this being everything here I wonder if this can't be folded with the
> patch where the function is introduced.

Sure, it can be folded. I will do that to reduce patch series.

~ Oleksii

Re: [PATCH v1 13/14] xen/riscv: initialize interrupt controller

Posted by Oleksii Kurochko 7 months, 2 weeks ago

On 4/17/25 12:11 PM, Oleksii Kurochko wrote:
>
>
> On 4/15/25 5:59 PM, Jan Beulich wrote:
>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>> Call intc_init() to do basic initialization steps for APLIC and IMISC.
>>>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> Acked-by: Jan Beulich<jbeulich@suse.com>
>> yet ...
>>
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>>   
>>>       intc_preinit();
>>>   
>>> +    intc_init();
>>> +
>>>       printk("All set up\n");
>>>   
>>>       machine_halt();
>> ... this being everything here I wonder if this can't be folded with the
>> patch where the function is introduced.
> Sure, it can be folded. I will do that to reduce patch series.

I doubled checked and, at the moment, when intc_init() is introduced:
void __init intc_init(void)
{
     ASSERT(intc_hw_ops);

     if ( intc_hw_ops->init() )
         panic("Failed to initialize the interrupt controller drivers\n");
}

intc_hw_ops isn't registered as they are registered in the next two patches after
intriduction of intc_hw_ops.

~ Oleksii

Re: [PATCH v1 13/14] xen/riscv: initialize interrupt controller

Posted by Jan Beulich 7 months, 2 weeks ago

On 30.04.2025 17:34, Oleksii Kurochko wrote:
> 
> On 4/17/25 12:11 PM, Oleksii Kurochko wrote:
>>
>>
>> On 4/15/25 5:59 PM, Jan Beulich wrote:
>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>> Call intc_init() to do basic initialization steps for APLIC and IMISC.
>>>>
>>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>>> Acked-by: Jan Beulich<jbeulich@suse.com>
>>> yet ...
>>>
>>>> --- a/xen/arch/riscv/setup.c
>>>> +++ b/xen/arch/riscv/setup.c
>>>> @@ -136,6 +136,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>>>   
>>>>       intc_preinit();
>>>>   
>>>> +    intc_init();
>>>> +
>>>>       printk("All set up\n");
>>>>   
>>>>       machine_halt();
>>> ... this being everything here I wonder if this can't be folded with the
>>> patch where the function is introduced.
>> Sure, it can be folded. I will do that to reduce patch series.
> 
> I doubled checked and, at the moment, when intc_init() is introduced:
> void __init intc_init(void)
> {
>      ASSERT(intc_hw_ops);
> 
>      if ( intc_hw_ops->init() )
>          panic("Failed to initialize the interrupt controller drivers\n");
> }
> 
> intc_hw_ops isn't registered as they are registered in the next two patches after
> intriduction of intc_hw_ops.

Which then feels wrong anyway; you're then merely leveraging that the function
has no caller, which (as said elsewhere) shouldn't be the case at the very least
for Misra's sake. So I expect some re-ordering to be necessary. Or you may want
to introduce the function empty and add the intc_hw_ops uses as intc_hw_ops is
introduced.

Jan