Re: [PATCH v1 09/27] xen/riscv: implement make_intc_domU_node()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2026 16:00, Oleksii Kurochko wrote:
> On 4/1/26 4:38 PM, Jan Beulich wrote:
>> On 10.03.2026 18:08, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/dom0less-build.c
>>> +++ b/xen/arch/riscv/dom0less-build.c
>>> @@ -3,6 +3,15 @@
>>>   #include <xen/fdt-kernel.h>
>>>   #include <xen/init.h>
>>>   
>>> +#include <asm/intc.h>
>>> +
>>> +int __init make_intc_domU_node(struct kernel_info *kinfo)
>>> +{
>>> +    intc_make_domu_dt_node(kinfo);
>>> +
>>> +    return 0;
>>> +}
>>
>> Is this wrapper really needed? Can't what's intc_make_domu_dt_node() right
>> now become make_intc_domU_node()?
> 
> With current implementation no as intc_hw_ops used inside 
> intc_make_domu_dt_node() is declared as static.

How does that matter if you simply rename intc_make_domu_dt_node()?

> But I can introduce:
> 
> enum intc_version intc_hw_version(void)
> {
>     return intc_hw_ops->info->hw_version;
> }
> 
> and the in make_intc_domU_node() just use switch/case to call interrupt 
> controller specific functions.
> 
> Would it be better? It will also help to ...
> 
>>
>>> @@ -41,6 +41,10 @@ struct intc_hw_operations {
>>>   
>>>       /* handle external interrupt */
>>>       void (*handle_interrupt)(struct cpu_user_regs *regs);
>>> +
>>> +    /* Create interrupt controller node for domain */
>>> +    int (*make_dom_dt_node)(const struct kernel_info *kinfo,
>>> +                            const struct dt_device_node *intc);
>>
>> An __init-only hook is somewhat risky, just to mention it. In IOMMU code
>> besides struct iommu_ops we have struct iommu_init_ops, just to give an
>> example of where the same could have been the case.
> 
> .. not introduce hooks in this structure which won't exist after init.

That would be nice (as already said).

>>> --- a/xen/arch/riscv/intc.c
>>> +++ b/xen/arch/riscv/intc.c
>>> @@ -67,3 +67,11 @@ void intc_route_irq_to_xen(struct irq_desc *desc, 
>>> unsigned int priority)
>>>       intc_set_irq_type(desc, desc->arch.type);
>>>       intc_set_irq_priority(desc, priority);
>>>   }
>>> +
>>> +int __init intc_make_domu_dt_node(const struct kernel_info *kinfo)
>>> +{
>>> +    if ( intc_hw_ops && intc_hw_ops->make_dom_dt_node )
>>> +        return intc_hw_ops->make_dom_dt_node(kinfo, 
>>> intc_hw_ops->info->node);
>>> +
>>> +    return -ENOSYS;
>>
>> How do you justify this choice of return value? This isn't even a hypercall
>> handler.
> 
> That make_dom_dt_node() isn't provided by interrupt controller, so isn't 
> supported or as it mentioned in the comment "not implemented in 
> public/errno.h:
>    XEN_ERRNO(ENOSYS,  38)     /* Function not implemented */

"Function" as in "system call function" (normally; for us: "hypercall 
function").
ENOSYS really has a pretty narrow range of valid uses (according to my reading).
EOPNOTSUPP is the more generic alternative.

Jan