| Message ID | 0b4d49742f58549ec644944ce1e02c98d7551845.1726579819.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Move {acpi_}device_init() and device_get_class() to common code | expand |
On 17.09.2024 18:15, Oleksii Kurochko wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -12,6 +12,14 @@ config CORE_PARKING > bool > depends on NR_CPUS > 1 > > +config DEVICE_INIT > + bool > + default !X86 This can simply be "def_bool y" as ... > + depends on !X86 && (ACPI || HAS_DEVICE_TREE) ... this enforces all restrictions. As indicated before I'd prefer if we could get away without yet another Kconfig constant, which would then also eliminate my concern about the expression not really covering for the case where x86 would obtain DT support (and hence likely needing the initialization here, too). What about ... > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_HYPFS_CONFIG) += config_data.o > obj-$(CONFIG_CORE_PARKING) += core_parking.o > obj-y += cpu.o > obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o > +obj-$(CONFIG_DEVICE_INIT) += device.o obj-$(CONFIG_HAS_DEVICE_TREE) += device.o obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o ? (Eventually we could then simplify this to just obj-$(CONFIG_ACPI), to allow DT on x86, making sure the ACPI part of the file builds for x86 but does nothing there.) Jan
On Mon, 2024-09-23 at 16:43 +0200, Jan Beulich wrote: > On 17.09.2024 18:15, Oleksii Kurochko wrote: > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -12,6 +12,14 @@ config CORE_PARKING > > bool > > depends on NR_CPUS > 1 > > > > +config DEVICE_INIT > > + bool > > + default !X86 > > This can simply be "def_bool y" as ... > > > + depends on !X86 && (ACPI || HAS_DEVICE_TREE) > > ... this enforces all restrictions. As indicated before I'd prefer if > we > could get away without yet another Kconfig constant, which would then > also eliminate my concern about the expression not really covering > for > the case where x86 would obtain DT support (and hence likely needing > the > initialization here, too). What about ... > > > --- a/xen/common/Makefile > > +++ b/xen/common/Makefile > > @@ -6,6 +6,7 @@ obj-$(CONFIG_HYPFS_CONFIG) += config_data.o > > obj-$(CONFIG_CORE_PARKING) += core_parking.o > > obj-y += cpu.o > > obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o > > +obj-$(CONFIG_DEVICE_INIT) += device.o > > obj-$(CONFIG_HAS_DEVICE_TREE) += device.o > obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o > > ? (Eventually we could then simplify this to just obj-$(CONFIG_ACPI), > to allow DT on x86, making sure the ACPI part of the file builds for > x86 but does nothing there.) I am okay with this solution. It seems I misunderstood you in the first version of this patch series. When "obj-$(or ... )" was used, I decided it wasn’t a good approach to use 'or', 'filter-out', or other similar functions in Makefiles for such cases. I couldn't come up with a better solution at the time, so I introduced a new config instead. The only issue I see with this approach is that in device.c, there is the following code: #ifdef CONFIG_ACPI extern const struct acpi_device_desc _asdevice[], _aedevice[]; int __init acpi_device_init(enum device_class class, const void *data, int class_type) ... #endif This shouldn't be compiled for X86. However, it will still be compiled if we simplify to: obj-$(CONFIG_HAS_DEVICE_TREE) += device.o obj-$(CONFIG_ACPI) += device.o The situation where we have CONFIG_HAS_DEVICE_TREE=y and CONFIG_ACPI=y is possible ( if X86 will start or already using CONFIG_HAS_DEVICE_TREE ). The same will be if the second obj looks like: "obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o" and X86 will use CONFIG_HAS_DEVICE_TREE. To resolve this, we probably need two separate files: device-dt.c and device-acpi.c, and then use: obj-$(CONFIG_HAS_DEVICE_TREE) += device-dt.o obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device-acpi.o Alternatively, we could make an exception in device.c and add a !CONFIG_X86 condition: #if defined(CONFIG_ACPI) && !defined(CONFIG_X86) extern const struct acpi_device_desc _asdevice[], _aedevice[]; int __init acpi_device_init(enum device_class class, const void *data, int class_type) ... #endif ~ Oleksii
On 24.09.2024 12:16, oleksii.kurochko@gmail.com wrote: > On Mon, 2024-09-23 at 16:43 +0200, Jan Beulich wrote: >> On 17.09.2024 18:15, Oleksii Kurochko wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -12,6 +12,14 @@ config CORE_PARKING >>> bool >>> depends on NR_CPUS > 1 >>> >>> +config DEVICE_INIT >>> + bool >>> + default !X86 >> >> This can simply be "def_bool y" as ... >> >>> + depends on !X86 && (ACPI || HAS_DEVICE_TREE) >> >> ... this enforces all restrictions. As indicated before I'd prefer if >> we >> could get away without yet another Kconfig constant, which would then >> also eliminate my concern about the expression not really covering >> for >> the case where x86 would obtain DT support (and hence likely needing >> the >> initialization here, too). What about ... >> >>> --- a/xen/common/Makefile >>> +++ b/xen/common/Makefile >>> @@ -6,6 +6,7 @@ obj-$(CONFIG_HYPFS_CONFIG) += config_data.o >>> obj-$(CONFIG_CORE_PARKING) += core_parking.o >>> obj-y += cpu.o >>> obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o >>> +obj-$(CONFIG_DEVICE_INIT) += device.o >> >> obj-$(CONFIG_HAS_DEVICE_TREE) += device.o >> obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >> >> ? (Eventually we could then simplify this to just obj-$(CONFIG_ACPI), >> to allow DT on x86, making sure the ACPI part of the file builds for >> x86 but does nothing there.) > I am okay with this solution. It seems I misunderstood you in the first > version of this patch series. When "obj-$(or ... )" was used, I decided > it wasn’t a good approach to use 'or', 'filter-out', or other similar > functions in Makefiles for such cases. I couldn't come up with a better > solution at the time, so I introduced a new config instead. > > The only issue I see with this approach is that in device.c, there is > the following code: > #ifdef CONFIG_ACPI > > extern const struct acpi_device_desc _asdevice[], _aedevice[]; > > int __init acpi_device_init(enum device_class class, const void *data, > int class_type) > ... > > #endif > This shouldn't be compiled for X86. However, it will still be compiled > if we simplify to: > obj-$(CONFIG_HAS_DEVICE_TREE) += device.o > obj-$(CONFIG_ACPI) += device.o Which is why I said "eventually". For now it'll be the $(filter-out ...). How to best adjust the code to permit above simplification is to be determined; what you outlined matches what I had thought of as an option. Jan
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index 3e02cff008..5610cddcba 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -8,79 +8,12 @@ * Copyright (C) 2013 Linaro Limited. */ -#include <asm/device.h> -#include <asm/setup.h> +#include <xen/device_tree.h> #include <xen/errno.h> -#include <xen/init.h> #include <xen/iocap.h> #include <xen/lib.h> -extern const struct device_desc _sdevice[], _edevice[]; - -#ifdef CONFIG_ACPI -extern const struct acpi_device_desc _asdevice[], _aedevice[]; -#endif - -int __init device_init(struct dt_device_node *dev, enum device_class class, - const void *data) -{ - const struct device_desc *desc; - - ASSERT(dev != NULL); - - if ( !dt_device_is_available(dev) || dt_device_for_passthrough(dev) ) - return -ENODEV; - - for ( desc = _sdevice; desc != _edevice; desc++ ) - { - if ( desc->class != class ) - continue; - - if ( dt_match_node(desc->dt_match, dev) ) - { - ASSERT(desc->init != NULL); - - return desc->init(dev, data); - } - - } - - return -EBADF; -} - -#ifdef CONFIG_ACPI -int __init acpi_device_init(enum device_class class, const void *data, int class_type) -{ - const struct acpi_device_desc *desc; - - for ( desc = _asdevice; desc != _aedevice; desc++ ) - { - if ( ( desc->class != class ) || ( desc->class_type != class_type ) ) - continue; - - ASSERT(desc->init != NULL); - - return desc->init(data); - } - - return -EBADF; -} -#endif - -enum device_class device_get_class(const struct dt_device_node *dev) -{ - const struct device_desc *desc; - - ASSERT(dev != NULL); - - for ( desc = _sdevice; desc != _edevice; desc++ ) - { - if ( dt_match_node(desc->dt_match, dev) ) - return desc->class; - } - - return DEVICE_UNKNOWN; -} +#include <asm/setup.h> int map_irq_to_domain(struct domain *d, unsigned int irq, bool need_mapping, const char *devname) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 90268d9249..6c6ad30d99 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -12,6 +12,14 @@ config CORE_PARKING bool depends on NR_CPUS > 1 +config DEVICE_INIT + bool + default !X86 + depends on !X86 && (ACPI || HAS_DEVICE_TREE) + help + Enable support for device initialization using device tree or ACPI. + It is not available for X86 systems. + config GRANT_TABLE bool "Grant table support" if EXPERT default y diff --git a/xen/common/Makefile b/xen/common/Makefile index fc52e0857d..1d5a4bb18d 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_HYPFS_CONFIG) += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o +obj-$(CONFIG_DEVICE_INIT) += device.o obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ obj-$(CONFIG_IOREQ_SERVER) += dm.o obj-y += domain.o diff --git a/xen/common/device.c b/xen/common/device.c new file mode 100644 index 0000000000..33e0d58f2f --- /dev/null +++ b/xen/common/device.c @@ -0,0 +1,82 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Based on the code from: + * xen/arch/arm/device.c + */ + +#include <xen/bug.h> +#include <xen/device_tree.h> +#include <xen/errno.h> +#include <xen/init.h> + +#include <asm/device.h> + +#ifdef CONFIG_HAS_DEVICE_TREE + +extern const struct device_desc _sdevice[], _edevice[]; + +int __init device_init(struct dt_device_node *dev, enum device_class class, + const void *data) +{ + const struct device_desc *desc; + + ASSERT(dev != NULL); + + if ( !dt_device_is_available(dev) || dt_device_for_passthrough(dev) ) + return -ENODEV; + + for ( desc = _sdevice; desc != _edevice; desc++ ) + { + if ( desc->class != class ) + continue; + + if ( dt_match_node(desc->dt_match, dev) ) + { + ASSERT(desc->init != NULL); + + return desc->init(dev, data); + } + } + + return -EBADF; +} + +enum device_class device_get_class(const struct dt_device_node *dev) +{ + const struct device_desc *desc; + + ASSERT(dev != NULL); + + for ( desc = _sdevice; desc != _edevice; desc++ ) + { + if ( dt_match_node(desc->dt_match, dev) ) + return desc->class; + } + + return DEVICE_UNKNOWN; +} + +#endif + +#ifdef CONFIG_ACPI + +extern const struct acpi_device_desc _asdevice[], _aedevice[]; + +int __init acpi_device_init(enum device_class class, const void *data, int class_type) +{ + const struct acpi_device_desc *desc; + + for ( desc = _asdevice; desc != _aedevice; desc++ ) + { + if ( ( desc->class != class ) || ( desc->class_type != class_type ) ) + continue; + + ASSERT(desc->init != NULL); + + return desc->init(data); + } + + return -EBADF; +} + +#endif
Remove the device initialization code from `xen/arch/arm/device.c` and move it to the common code to avoid duplication and make it accessible for both ARM and other architectures. device_get_class(), device_init(), _sdevice[] and _edevice[] are wrapped by "#ifdef CONFIG_HAS_DEVICE_TREE" for the case if an arch doesn't support device tree. Remove unnecessary inclusions of <asm/device.h> and <xen/init.h> from `xen/arch/arm/device.c` as no code in the file relies on these headers. Fix the inclusion order by moving <asm/setup.h> after <xen/*> headers to resolve a compilation error: ./include/public/xen.h:968:35: error: unknown type name 'uint64_t' 968 | __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); | ^~~~~~~~ ./include/public/arch-arm.h:191:21: note: in definition of macro '___DEFINE_XEN_GUEST_HANDLE' 191 | typedef union { type *p; uint64_aligned_t q; } \ | ^~~~ ./include/public/xen.h:968:1: note: in expansion of macro '__DEFINE_XEN_GUEST_HANDLE' 968 | __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); because <asm/setup.h> includes <public/version.h>, which in turn includes "xen.h", which requires <xen/types.h> to be processed correctly. Additionally, add <xen/device_tree.h> to `device.c` as functions from this header are used within the file. Introduce new CONFIG_DEVICE_INIT to compile `device.o` conditionally, ensuring the correct device initialization code is included as needed. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v2: - Introduce CONFIG_DEVICE_INIT to make common/device.o compilable only for Arm, PPC and RIS-V. - Move declaration of acpi_device_desc _asdevice[], _aedevice[] inside #ifdef CONFIG_ACPI ... #endif. - Include <asm/device.h> instead of <asm-generic/device.h> as <asm/device.h> will be included <asm-generic/device.h> anyway. --- xen/arch/arm/device.c | 71 ++----------------------------------- xen/common/Kconfig | 8 +++++ xen/common/Makefile | 1 + xen/common/device.c | 82 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 69 deletions(-) create mode 100644 xen/common/device.c