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

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-11-21 at 11:27 +0100, Michal Orzel wrote:
> 
> 
> 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.
I think you are right and we can really drop stub and guards. I'll send
a new patch version applying this and the comments above.

Thanks.

~ Oleksii