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

[Jan Beulich <jbeulich@xxxxxxxx>]

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