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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 19/11/2024 15:56, 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 v4:
>  - Add SPDX tag in intc.c
>  - s/num_gics/num_intc
>  - Update the comment: s/GIC/interrupt controller.
> ---
> 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   | 35 +++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h   |  6 ++++++
>  4 files changed, 43 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..d2bcbc2d5e
> --- /dev/null
> +++ b/xen/common/device-tree/intc.c
> @@ -0,0 +1,35 @@
> +/* 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_intc = 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 interrupt controller is supported */
> +            num_intc = 1;
> +            break;
> +        }
> +    }
> +
> +    if ( !num_intc )
> +        panic("Unable to find compatible interrupt contoller"
> +              "in the device tree\n");
Don't split printk messages. Also the split is incorrect as it'll result in 
"contollerin" (i.e. no space in between).
Also s/contoller/controller/
 
> +
> +    /* Set the interrupt controller as the primary interrupt controller */
> +    dt_interrupt_controller = node;
> +    dt_device_set_used_by(node, DOMID_XEN);
> +}
Missing EMACS block at the end of file.

> 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
Is it really needed to provide the stub and guards? Other DT related functions 
in this header are not
protected and AFAICT the inclusion of this header only works if 
CONFIG_HAS_DEVICE_TREE=y.

~Michal