Re: [PATCH v3] xen/common: Move gic_dt_preinit() to common code

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/11/2024 12:55, Oleksii Kurochko wrote:
> Introduce intc_dt_preinit() in the common codebase, as it is not
> architecture-specific and can be reused by both PPC and RISC-V.
> This function identifies the node with the interrupt-controller property
> in the device tree and calls device_init() to handle architecture-specific
> initialization of the interrupt controller.
>
> Make minor adjustments compared to the original ARM implementation of
> gic_dt_preinit():
>   - Remove the local rc variable in gic_dt_preinit() since it is only used once.
>   - Change the prefix from gic to intc to clarify that the function is not
>     specific to ARM’s GIC, making it suitable for other architectures as well.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v3:
>   - s/ic/intc.
>   - Update the commit message.
>   - Move intc_dt_preinit() to common/device-tree/intc.c.
>   - Add declaration of intc_dt_preinit() in xen/device_tree.h.
>   - Revert intc_preinit()-related changes and just back gic_preinit() in
>     Arm's gic.c.
>   - Revert ACPI-related changes.
> ---
> Changes in v2:
>   - Revert changes connected to moving of gic_acpi_preinit() to common code as
>     it isn't really architecture indepent part.
>   - Update the commit message.
>   - Move stub of ic_acpi_preinit() to <asm-generic/device.h> for the case when
>     CONFIG_ACPI=n.
> ---
>   xen/arch/arm/gic.c              | 32 +------------------------------
>   xen/common/device-tree/Makefile |  1 +
>   xen/common/device-tree/intc.c   | 34 +++++++++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h   |  6 ++++++
>   4 files changed, 42 insertions(+), 31 deletions(-)
>   create mode 100644 xen/common/device-tree/intc.c
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3eaf670fd7..acf61a4de3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -214,36 +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)
>   {
> @@ -269,7 +239,7 @@ static void __init gic_acpi_preinit(void) { }
>   void __init gic_preinit(void)
>   {
>       if ( acpi_disabled )
> -        gic_dt_preinit();
> +        intc_dt_preinit();
>       else
>           gic_acpi_preinit();
>   }
> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> index 58052d074e..7c549be38a 100644
> --- a/xen/common/device-tree/Makefile
> +++ b/xen/common/device-tree/Makefile
> @@ -2,3 +2,4 @@ obj-y += bootfdt.init.o
>   obj-y += bootinfo.init.o
>   obj-y += device-tree.o
>   obj-$(CONFIG_OVERLAY_DTB) += dt-overlay.o
> +obj-y += intc.o
> diff --git a/xen/common/device-tree/intc.c b/xen/common/device-tree/intc.c
> new file mode 100644
> index 0000000000..e726daeb26
> --- /dev/null
> +++ b/xen/common/device-tree/intc.c
> @@ -0,0 +1,34 @@
> +
Missing SPDX. Because this code is not brand new, the license will 
derive from gic.c which is unfortunately GPLv2+. So it would need to be:
/* SPDX-License-Identifier: GPL-2.0-or-later */

> +#include <xen/device_tree.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +
> +void __init intc_dt_preinit(void)
> +{
> +    struct dt_device_node *node;
> +    uint8_t num_gics = 0;
I think you want to rename this variable to num_intc.

> +
> +    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 */
And this probably need to be updated to "Only one interrupt controlled 
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);
> +}
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index e6287305a7..33d70b9594 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -238,6 +238,12 @@ extern rwlock_t dt_host_lock;
>   struct dt_device_node *
>   dt_find_interrupt_controller(const struct dt_device_match *matches);
>   
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +void intc_dt_preinit(void);
> +#else
> +static inline void intc_dt_preinit(void) { }
> +#endif
> +
>   #define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
>   #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
>   #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
Cheers,

--
Julien Grall