Introduce ic_preinit() in common code, as it is not architecture-specific
and can be reused by both PPC and RISC-V. The function identifies the node
with the interrupt-controller property and calls device_init() for
architecture-specific initialization of the interrupt controller when
using the device tree. And the similar is done in case of ACPI but instead of
searching of the interrupt-controller property it is searching for entry in
MADT and call acpi_device_init().

This patch makes minor changes compared to the Arm implementation:
- Removes the local rc variable in gic_dt_preinit() since it is only used once.
- Changes the prefix from gic to ic to generalize the code and avoid confusion
  with Arm’s GIC.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/arm/gic.c               | 60 -----------------------------
 xen/arch/arm/setup.c             |  3 +-
 xen/common/device.c              | 65 ++++++++++++++++++++++++++++++++
 xen/include/asm-generic/device.h |  2 +
 4 files changed, 69 insertions(+), 61 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3eaf670fd7..b18bb08eba 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -214,66 +214,6 @@ int gic_map_hwdom_extra_mappings(struct domain *d)
     return 0;
 }
 
-static void __init gic_dt_preinit(void)
-{
-    int rc;
-    struct dt_device_node *node;
-    uint8_t num_gics = 0;
-
-    dt_for_each_device_node( dt_host, node )
-    {
-        if ( !dt_get_property(node, "interrupt-controller", NULL) )
-            continue;
-
-        if ( !dt_get_parent(node) )
-            continue;
-
-        rc = device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL);
-        if ( !rc )
-        {
-            /* NOTE: Only one GIC is supported */
-            num_gics = 1;
-            break;
-        }
-    }
-    if ( !num_gics )
-        panic("Unable to find compatible GIC in the device tree\n");
-
-    /* Set the GIC as the primary interrupt controller */
-    dt_interrupt_controller = node;
-    dt_device_set_used_by(node, DOMID_XEN);
-}
-
-#ifdef CONFIG_ACPI
-static void __init gic_acpi_preinit(void)
-{
-    struct acpi_subtable_header *header;
-    struct acpi_madt_generic_distributor *dist;
-
-    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
-    if ( !header )
-        panic("No valid GICD entries exists\n");
-
-    dist = container_of(header, struct acpi_madt_generic_distributor, header);
-
-    if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist->version) )
-        panic("Unable to find compatible GIC in the ACPI table\n");
-}
-#else
-static void __init gic_acpi_preinit(void) { }
-#endif
-
-/* Find the interrupt controller and set up the callback to translate
- * device tree or ACPI IRQ.
- */
-void __init gic_preinit(void)
-{
-    if ( acpi_disabled )
-        gic_dt_preinit();
-    else
-        gic_acpi_preinit();
-}
-
 /* Set up the GIC */
 void __init gic_init(void)
 {
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca..1ea7db0bd4 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -38,6 +38,7 @@
 #include <asm/page.h>
 #include <asm/static-evtchn.h>
 #include <asm/current.h>
+#include <asm/device.h>
 #include <asm/setup.h>
 #include <asm/gic.h>
 #include <asm/cpuerrata.h>
@@ -359,7 +360,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
 
     preinit_xen_time();
 
-    gic_preinit();
+    ic_preinit();
 
     arm_uart_init();
     console_init_preirq();
diff --git a/xen/common/device.c b/xen/common/device.c
index 33e0d58f2f..cb07bd6e4f 100644
--- a/xen/common/device.c
+++ b/xen/common/device.c
@@ -4,10 +4,14 @@
  *   xen/arch/arm/device.c
  */
 
+#include <xen/acpi.h>
 #include <xen/bug.h>
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/kernel.h>
+#include <xen/lib.h>
+#include <xen/types.h>
 
 #include <asm/device.h>
 
@@ -56,6 +60,40 @@ enum device_class device_get_class(const struct dt_device_node *dev)
     return DEVICE_UNKNOWN;
 }
 
+static void __init ic_dt_preinit(void)
+{
+    struct dt_device_node *node;
+    uint8_t num_gics = 0;
+
+    dt_for_each_device_node( dt_host, node )
+    {
+        if ( !dt_get_property(node, "interrupt-controller", NULL) )
+            continue;
+
+        if ( !dt_get_parent(node) )
+            continue;
+
+        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL) )
+        {
+            /* NOTE: Only one GIC is supported */
+            num_gics = 1;
+            break;
+        }
+    }
+
+    if ( !num_gics )
+        panic("Unable to find compatible interrupt contoller"
+              "in the device tree\n");
+
+    /* Set the interrupt controller as the primary interrupt controller */
+    dt_interrupt_controller = node;
+    dt_device_set_used_by(node, DOMID_XEN);
+}
+
+#else /* !CONFIG_HAS_DEVICE_TREE */
+
+static void __init ic_dt_preinit(void) { }
+
 #endif
 
 #ifdef CONFIG_ACPI
@@ -79,4 +117,31 @@ int __init acpi_device_init(enum device_class class, const void *data, int class
     return -EBADF;
 }
 
+static void __init ic_acpi_preinit(void)
+{
+    struct acpi_subtable_header *header;
+    struct acpi_madt_generic_distributor *dist;
+
+    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
+    if ( !header )
+        panic("No valid interrupt controller entries exists\n");
+
+    dist = container_of(header, struct acpi_madt_generic_distributor, header);
+
+    if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist->version) )
+        panic("Unable to find compatible interrupt controller"
+              "in the ACPI table\n");
+}
+#else /* !CONFIG_ACPI */
+
+static void __init ic_acpi_preinit(void) { }
+
 #endif
+
+void __init ic_preinit(void)
+{
+    if ( acpi_disabled )
+        ic_dt_preinit();
+    else
+        ic_acpi_preinit();
+}
diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
index 1acd1ba1d8..ccfb062d6a 100644
--- a/xen/include/asm-generic/device.h
+++ b/xen/include/asm-generic/device.h
@@ -127,6 +127,8 @@ __section(".adev.info") = {                                         \
 
 #endif /* CONFIG_ACPI */
 
+void ic_preinit(void);
+
 #endif /* __ASM_GENERIC_DEVICE_H__ */
 
 /*
-- 
2.47.0

On 29.10.2024 17:47, Oleksii Kurochko wrote:
> --- a/xen/common/device.c
> +++ b/xen/common/device.c
> @@ -4,10 +4,14 @@
>   *   xen/arch/arm/device.c
>   */
>  
> +#include <xen/acpi.h>
>  #include <xen/bug.h>
>  #include <xen/device_tree.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
> +#include <xen/kernel.h>
> +#include <xen/lib.h>
> +#include <xen/types.h>

I don't think non-ACPI configs should include xen/acpi.h here. Surely this
can be moved down into the ACPI-only code section?

> @@ -56,6 +60,40 @@ enum device_class device_get_class(const struct dt_device_node *dev)
>      return DEVICE_UNKNOWN;
>  }
>  
> +static void __init ic_dt_preinit(void)
> +{
> +    struct dt_device_node *node;
> +    uint8_t num_gics = 0;
> +
> +    dt_for_each_device_node( dt_host, node )
> +    {
> +        if ( !dt_get_property(node, "interrupt-controller", NULL) )
> +            continue;
> +
> +        if ( !dt_get_parent(node) )
> +            continue;
> +
> +        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL) )
> +        {
> +            /* NOTE: Only one GIC is supported */
> +            num_gics = 1;
> +            break;
> +        }
> +    }
> +
> +    if ( !num_gics )
> +        panic("Unable to find compatible interrupt contoller"
> +              "in the device tree\n");
> +
> +    /* Set the interrupt controller as the primary interrupt controller */
> +    dt_interrupt_controller = node;
> +    dt_device_set_used_by(node, DOMID_XEN);
> +}
> +
> +#else /* !CONFIG_HAS_DEVICE_TREE */
> +
> +static void __init ic_dt_preinit(void) { }
> +
>  #endif

While for DT I can only guess that the IC is common across platforms, ...

> @@ -79,4 +117,31 @@ int __init acpi_device_init(enum device_class class, const void *data, int class
>      return -EBADF;
>  }
>  
> +static void __init ic_acpi_preinit(void)
> +{
> +    struct acpi_subtable_header *header;
> +    struct acpi_madt_generic_distributor *dist;
> +
> +    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> +    if ( !header )
> +        panic("No valid interrupt controller entries exists\n");
> +
> +    dist = container_of(header, struct acpi_madt_generic_distributor, header);
> +
> +    if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist->version) )
> +        panic("Unable to find compatible interrupt controller"
> +              "in the ACPI table\n");
> +}
> +#else /* !CONFIG_ACPI */
> +
> +static void __init ic_acpi_preinit(void) { }
> +
>  #endif

... simply deriving from x86 (and IA-64) it's clear it isn't for ACPI. I'm
therefore unconvinced placing this in common code is appropriate.

Jan

On Tue, 2024-10-29 at 17:57 +0100, Jan Beulich wrote:
> On 29.10.2024 17:47, Oleksii Kurochko wrote:
> > --- a/xen/common/device.c
> > +++ b/xen/common/device.c
> > @@ -4,10 +4,14 @@
> >    *   xen/arch/arm/device.c
> >    */
> >   
> > +#include <xen/acpi.h>
> >   #include <xen/bug.h>
> >   #include <xen/device_tree.h>
> >   #include <xen/errno.h>
> >   #include <xen/init.h>
> > +#include <xen/kernel.h>
> > +#include <xen/lib.h>
> > +#include <xen/types.h>
> 
> I don't think non-ACPI configs should include xen/acpi.h here. Surely
> this
> can be moved down into the ACPI-only code section?
xen/acpi.h was included as acpi_disabled is defined there and it is
needed in commom/device.c for:
```
void __init ic_preinit(void)
{
    if ( acpi_disabled )
        ic_dt_preinit();
    else
        ic_acpi_preinit();
}
```
It seems to me that ic_preinit() could be also in generic code and just
ic_acpi_preinit() to be defined in architecture specific code.

~ Oleksii

On 30.10.2024 10:50, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-10-29 at 17:57 +0100, Jan Beulich wrote:
>> On 29.10.2024 17:47, Oleksii Kurochko wrote:
>>> --- a/xen/common/device.c
>>> +++ b/xen/common/device.c
>>> @@ -4,10 +4,14 @@
>>>    *   xen/arch/arm/device.c
>>>    */
>>>   
>>> +#include <xen/acpi.h>
>>>   #include <xen/bug.h>
>>>   #include <xen/device_tree.h>
>>>   #include <xen/errno.h>
>>>   #include <xen/init.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/types.h>
>>
>> I don't think non-ACPI configs should include xen/acpi.h here. Surely
>> this
>> can be moved down into the ACPI-only code section?
> xen/acpi.h was included as acpi_disabled is defined there and it is
> needed in commom/device.c for:
> ```
> void __init ic_preinit(void)
> {
>     if ( acpi_disabled )
>         ic_dt_preinit();
>     else
>         ic_acpi_preinit();
> }
> ```

Oh, I see.

Jan

On Tue, 2024-10-29 at 17:57 +0100, Jan Beulich wrote:
> On 29.10.2024 17:47, Oleksii Kurochko wrote:
> > --- a/xen/common/device.c
> > +++ b/xen/common/device.c
> > @@ -4,10 +4,14 @@
> >   *   xen/arch/arm/device.c
> >   */
> >  
> > +#include <xen/acpi.h>
> >  #include <xen/bug.h>
> >  #include <xen/device_tree.h>
> >  #include <xen/errno.h>
> >  #include <xen/init.h>
> > +#include <xen/kernel.h>
> > +#include <xen/lib.h>
> > +#include <xen/types.h>
> 
> I don't think non-ACPI configs should include xen/acpi.h here. Surely
> this
> can be moved down into the ACPI-only code section?
> 
> > @@ -56,6 +60,40 @@ enum device_class device_get_class(const struct
> > dt_device_node *dev)
> >      return DEVICE_UNKNOWN;
> >  }
> >  
> > +static void __init ic_dt_preinit(void)
> > +{
> > +    struct dt_device_node *node;
> > +    uint8_t num_gics = 0;
> > +
> > +    dt_for_each_device_node( dt_host, node )
> > +    {
> > +        if ( !dt_get_property(node, "interrupt-controller", NULL)
> > )
> > +            continue;
> > +
> > +        if ( !dt_get_parent(node) )
> > +            continue;
> > +
> > +        if ( !device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL)
> > )
> > +        {
> > +            /* NOTE: Only one GIC is supported */
> > +            num_gics = 1;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !num_gics )
> > +        panic("Unable to find compatible interrupt contoller"
> > +              "in the device tree\n");
> > +
> > +    /* Set the interrupt controller as the primary interrupt
> > controller */
> > +    dt_interrupt_controller = node;
> > +    dt_device_set_used_by(node, DOMID_XEN);
> > +}
> > +
> > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > +
> > +static void __init ic_dt_preinit(void) { }
> > +
> >  #endif
> 
> While for DT I can only guess that the IC is common across platforms,
> ...
> 
> > @@ -79,4 +117,31 @@ int __init acpi_device_init(enum device_class
> > class, const void *data, int class
> >      return -EBADF;
> >  }
> >  
> > +static void __init ic_acpi_preinit(void)
> > +{
> > +    struct acpi_subtable_header *header;
> > +    struct acpi_madt_generic_distributor *dist;
> > +
> > +    header =
> > acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> > +    if ( !header )
> > +        panic("No valid interrupt controller entries exists\n");
> > +
> > +    dist = container_of(header, struct
> > acpi_madt_generic_distributor, header);
> > +
> > +    if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist-
> > >version) )
> > +        panic("Unable to find compatible interrupt controller"
> > +              "in the ACPI table\n");
> > +}
> > +#else /* !CONFIG_ACPI */
> > +
> > +static void __init ic_acpi_preinit(void) { }
> > +
> >  #endif
> 
> ... simply deriving from x86 (and IA-64) it's clear it isn't for
> ACPI. I'm
> therefore unconvinced placing this in common code is appropriate.
Agree, I don't check acpi_madt_generic_distributor and it is ARM
specific so I will drop it and leave only DT part.

Thanks.

~ Oleksii