Re: [PATCH v2 5/5] xen/common: move device initialization code to common code

[oleksii.kurochko@xxxxxxxxx]

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