| Message ID | b1ded26dcebf9a3228fe4062825273720e0c59fe.1730220384.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] xen/common: move gic_preinit() to common code | expand |
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
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
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? xen/acpi.h was included as acpi_disabled is defined there and it is needed in commom/device.c for: ``` void __init ic_preinit(void) { if ( acpi_disabled ) ic_dt_preinit(); else ic_acpi_preinit(); } ``` It seems to me that ic_preinit() could be also in generic code and just ic_acpi_preinit() to be defined in architecture specific code. ~ Oleksii
On 30.10.2024 10:50, oleksii.kurochko@gmail.com wrote: > 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? > xen/acpi.h was included as acpi_disabled is defined there and it is > needed in commom/device.c for: > ``` > void __init ic_preinit(void) > { > if ( acpi_disabled ) > ic_dt_preinit(); > else > ic_acpi_preinit(); > } > ``` Oh, I see. Jan
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 3eaf670fd7..b18bb08eba 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -214,66 +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) -{ - 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 GICD 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 GIC in the ACPI table\n"); -} -#else -static void __init gic_acpi_preinit(void) { } -#endif - -/* Find the interrupt controller and set up the callback to translate - * device tree or ACPI IRQ. - */ -void __init gic_preinit(void) -{ - if ( acpi_disabled ) - gic_dt_preinit(); - else - gic_acpi_preinit(); -} - /* Set up the GIC */ void __init gic_init(void) { diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 71ebaa77ca..1ea7db0bd4 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -38,6 +38,7 @@ #include <asm/page.h> #include <asm/static-evtchn.h> #include <asm/current.h> +#include <asm/device.h> #include <asm/setup.h> #include <asm/gic.h> #include <asm/cpuerrata.h> @@ -359,7 +360,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) preinit_xen_time(); - gic_preinit(); + ic_preinit(); arm_uart_init(); console_init_preirq(); diff --git a/xen/common/device.c b/xen/common/device.c index 33e0d58f2f..cb07bd6e4f 100644 --- 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> #include <asm/device.h> @@ -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 #ifdef CONFIG_ACPI @@ -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 + +void __init ic_preinit(void) +{ + if ( acpi_disabled ) + ic_dt_preinit(); + else + ic_acpi_preinit(); +} diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h index 1acd1ba1d8..ccfb062d6a 100644 --- a/xen/include/asm-generic/device.h +++ b/xen/include/asm-generic/device.h @@ -127,6 +127,8 @@ __section(".adev.info") = { \ #endif /* CONFIG_ACPI */ +void ic_preinit(void); + #endif /* __ASM_GENERIC_DEVICE_H__ */ /*
Introduce ic_preinit() in common code, as it is not architecture-specific and can be reused by both PPC and RISC-V. The function identifies the node with the interrupt-controller property and calls device_init() for architecture-specific initialization of the interrupt controller when using the device tree. And the similar is done in case of ACPI but instead of searching of the interrupt-controller property it is searching for entry in MADT and call acpi_device_init(). This patch makes minor changes compared to the Arm implementation: - Removes the local rc variable in gic_dt_preinit() since it is only used once. - Changes the prefix from gic to ic to generalize the code and avoid confusion with Arm’s GIC. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/arm/gic.c | 60 ----------------------------- xen/arch/arm/setup.c | 3 +- xen/common/device.c | 65 ++++++++++++++++++++++++++++++++ xen/include/asm-generic/device.h | 2 + 4 files changed, 69 insertions(+), 61 deletions(-)