Re: [PATCH v1] xen/common: move gic_preinit() to common code

[oleksii.kurochko@xxxxxxxxx]

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