[PATCH v4 0/6] Move {acpi_}device_init() and device_get_class() to common code

[PATCH v4 0/6] Move {acpi_}device_init() and device_get_class() to common code

[Oleksii Kurochko]

The {acpi_}device_init() and device_get_class() functions are identical across
Arm and RISC-V, and they are likely to remain the same for other architectures
like PPC.
Since there is no architecture-specific logic within these functions, they are
good candidates to be moved to the common codebase.

This patch series refactors the code by moving these functions to the common
directory, reducing code duplication and simplifying future maintenance.

---
Changes in V4:
 - Introduce SIMPLE_DECL_SECTION to cover the cases when an architecture wants
   to use section declaration without specifying an load address.
 - Refactor macors ACPI_DEV_INFO and DT_DEV_INFO and update their defintion.
---
Changes in V3:
 - drop _SECTIONS from the name of the macros ADEV_INFO and DT_DEV_INFO
 - update the definion of macros ADEV_INFO and DT_DEV_INFO
 - drop DEVICE_INIT config.
---
Changes in v2:
 - Introduce macros for definition of ACPI and Device Tree sections.
 - Introduce CONFIG_DEVICE_INIT to make common/device.o compilable only for Arm,
   PPC and RISC-V.
---

Oleksii Kurochko (6):
  xen: introduce SIMPLE_DECL_SECTION
  xen: define ACPI and DT device info sections macros
  xen/arm: use {DT,ACPI}_DEV_INFO for device info sections
  xen/ppc: add section for device information in linker script
  xen/riscv: add section for device information in linker script
  xen/common: move device initialization code to common code

 xen/arch/arm/device.c     | 71 +--------------------------------
 xen/arch/arm/xen.lds.S    | 16 ++------
 xen/arch/ppc/xen.lds.S    |  2 +
 xen/arch/riscv/xen.lds.S  |  4 ++
 xen/arch/x86/xen.lds.S    |  6 ++-
 xen/common/Makefile       |  2 +
 xen/common/device.c       | 82 +++++++++++++++++++++++++++++++++++++++
 xen/include/xen/xen.lds.h | 22 +++++++++++
 8 files changed, 122 insertions(+), 83 deletions(-)
 create mode 100644 xen/common/device.c

-- 
2.46.1

[PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Oleksii Kurochko]

Introduce SIMPLE_DECL_SECTION to cover the case when
an architecture wants to declare a section without specifying
of load address for the section.

Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - new patch
---
 xen/arch/x86/xen.lds.S    | 6 ++++--
 xen/include/xen/xen.lds.h | 6 ++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index b60d2f0d82..9275a566e1 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -3,6 +3,10 @@
 
 #include <xen/cache.h>
 #include <xen/lib.h>
+
+#ifdef EFI
+#define SIMPLE_DECL_SECTION
+#endif
 #include <xen/xen.lds.h>
 #include <asm/page.h>
 #undef ENTRY
@@ -12,9 +16,7 @@
 
 #define FORMAT "pei-x86-64"
 #undef __XEN_VIRT_START
-#undef DECL_SECTION
 #define __XEN_VIRT_START __image_base__
-#define DECL_SECTION(x) x :
 
 ENTRY(efi_start)
 
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 24b8900ffe..8135732756 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -5,6 +5,8 @@
  * Common macros to be used in architecture specific linker scripts.
  */
 
+#ifndef SIMPLE_DECL_SECTION
+
 /*
  * Declare a section whose load address is based at PA 0 rather than
  * Xen's virtual base address.
@@ -15,6 +17,10 @@
 # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
 #endif
 
+#else /* SIMPLE_DECL_SECTION */
+#define DECL_SECTION(x) x :
+#endif
+
 /*
  * To avoid any confusion, please note that the EFI macro does not correspond
  * to EFI support and is used when linking a native EFI (i.e. PE/COFF) binary,
-- 
2.46.1

[PATCH v4 2/6] xen: define ACPI and DT device info sections macros

[Oleksii Kurochko]

Introduce macros to define device information sections based on
the configuration of ACPI or device tree support. These sections
are required for common code of device initialization and getting
an information about a device.

These macros are expected to be used across different
architectures (Arm, PPC, RISC-V), so they are moved to
the common xen/xen.lds.h, based on their original definition
in Arm.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - rename ADEV_INFO to ACPI_DEV_INFO.
 - refactor ADEV_INFO and DT_DEV_INFO: add alignment and DECL_SECTION.
---
 Changes in V3:
  - drop SEC* at the end of ACPI AND DT device info
    section mancros.
  - refactor ADEV_INFO and DT_DEV_INFO macros.
---
 xen/include/xen/xen.lds.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 8135732756..cec86d0781 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -120,6 +120,14 @@
 
 /* List of constructs other than *_SECTIONS in alphabetical order. */
 
+#define ACPI_DEV_INFO(secname)  \
+  . = ALIGN(POINTER_ALIGN);     \
+  DECL_SECTION(secname) {       \
+      _asdevice = .;            \
+      *(secname)                \
+      _aedevice = .;            \
+  } :text
+
 #define BUGFRAMES                               \
     __start_bug_frames_0 = .;                   \
     *(.bug_frames.0)                            \
@@ -137,6 +145,14 @@
     *(.bug_frames.3)                            \
     __stop_bug_frames_3 = .;
 
+#define DT_DEV_INFO(secname)    \
+  . = ALIGN(POINTER_ALIGN);     \
+  DECL_SECTION(secname) {       \
+       _sdevice = .;           \
+       *(secname)              \
+       _edevice = .;           \
+  } :text
+
 #ifdef CONFIG_HYPFS
 #define HYPFS_PARAM              \
        . = ALIGN(POINTER_ALIGN); \
-- 
2.46.1

[PATCH v4 3/6] xen/arm: use {DT,ACPI}_DEV_INFO for device info sections

[Oleksii Kurochko]

Refactor arm/xen.lds.S by replacing the inline definitions for
device info sections with the newly introduced {DT,ACPI}_DEV_INFO
macros from xen/xen.lds.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - use newly refactored ACPI_DEV_INFO and DT_DEV_INFO
---
Changes in V3:
 - use refactored ADEV_INFO and DT_DEV_INFO macros.
---
 xen/arch/arm/xen.lds.S | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 0987052f1a..3b7b677197 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -4,6 +4,8 @@
 
 #include <xen/cache.h>
 #include <xen/lib.h>
+
+#define SIMPLE_DECL_SECTION
 #include <xen/xen.lds.h>
 #include <asm/page.h>
 #undef ENTRY
@@ -124,20 +126,10 @@ SECTIONS
       _eplatform = .;
   } :text
 
-  . = ALIGN(8);
-  .dev.info : {
-      _sdevice = .;
-      *(.dev.info)
-      _edevice = .;
-  } :text
+  DT_DEV_INFO(.dev.info)
 
 #ifdef CONFIG_ACPI
-  . = ALIGN(8);
-  .adev.info : {
-      _asdevice = .;
-      *(.adev.info)
-      _aedevice = .;
-  } :text
+  ACPI_DEV_INFO(adev.info)
 #endif
 
   . = ALIGN(8);
-- 
2.46.1

[PATCH v4 4/6] xen/ppc: add section for device information in linker script

[Oleksii Kurochko]

Introduce a new `.dev.info` section in the PPC linker script to
handle device-specific information. This section is required by
common code (common/device.c: device_init(), device_get_class() ).
This section is aligned to `POINTER_ALIGN`, with `_sdevice` and `_edevice`
marking the start and end of the section, respectively.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V4:
 - use newly refactored DT_DEV_INFO
---
Changes in V3:
 - use refactored DT_DEV_INFO macros.
 - Add Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 xen/arch/ppc/xen.lds.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
index 0c4b94814b..9222213484 100644
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -94,6 +94,8 @@ SECTIONS
         CONSTRUCTORS
     } :text
 
+    DT_DEV_INFO(.dev.info)            /* Devicetree based device info */
+
     . = ALIGN(PAGE_SIZE);             /* Init code and data */
     __init_begin = .;
     DECL_SECTION(.init.text) {
-- 
2.46.1

[PATCH v4 5/6] xen/riscv: add section for device information in linker script

[Oleksii Kurochko]

Introduce a new `.dev.info` section in the RISC-V linker script to
handle device-specific information. This section is required by
common code (common/device.c: device_init(), device_get_class() ).
This section is aligned to `POINTER_ALIGN`, with `_sdevice` and `_edevice`
marking the start and end of the section, respectively.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - use newly refactored DT_DEV_INFO
---
Changes in V3:
 - use refactored DT_DEV_INFO macros.
---
 xen/arch/riscv/xen.lds.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 871b47a235..c1400e0613 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -1,4 +1,6 @@
 #include <xen/lib.h>
+
+#define SIMPLE_DECL_SECTION
 #include <xen/xen.lds.h>
 
 OUTPUT_ARCH(riscv)
@@ -91,6 +93,8 @@ SECTIONS
         CONSTRUCTORS
     } :text
 
+    DT_DEV_INFO(.dev.info)            /* Devicetree based device info */
+
     . = ALIGN(PAGE_SIZE);             /* Init code and data */
     __init_begin = .;
     .init.text : {
-- 
2.46.1

[PATCH v4 6/6] xen/common: move device initialization code to common code

[Oleksii Kurochko]

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.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V4:
 - add Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V3:
 - drop DEVICE_INIT config.
 - update the commit message.
---
 xen/arch/arm/device.c | 71 ++-----------------------------------
 xen/common/Makefile   |  2 ++
 xen/common/device.c   | 82 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 69 deletions(-)
 create mode 100644 xen/common/device.c

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/Makefile b/xen/common/Makefile
index fc52e0857d..9d962069f7 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -6,6 +6,8 @@ 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_HAS_DEVICE_TREE) += device.o
+obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += 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
-- 
2.46.1

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Jan Beulich]

On 26.09.2024 18:54, Oleksii Kurochko wrote:
> Introduce SIMPLE_DECL_SECTION to cover the case when
> an architecture wants to declare a section without specifying
> of load address for the section.
> 
> Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v4 2/6] xen: define ACPI and DT device info sections macros

[Jan Beulich]

On 26.09.2024 18:54, Oleksii Kurochko wrote:
> Introduce macros to define device information sections based on
> the configuration of ACPI or device tree support. These sections
> are required for common code of device initialization and getting
> an information about a device.
> 
> These macros are expected to be used across different
> architectures (Arm, PPC, RISC-V), so they are moved to
> the common xen/xen.lds.h, based on their original definition
> in Arm.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with ...

> @@ -137,6 +145,14 @@
>      *(.bug_frames.3)                            \
>      __stop_bug_frames_3 = .;
>  
> +#define DT_DEV_INFO(secname)    \
> +  . = ALIGN(POINTER_ALIGN);     \
> +  DECL_SECTION(secname) {       \
> +       _sdevice = .;           \
> +       *(secname)              \
> +       _edevice = .;           \
> +  } :text

... the trailing backslashes all aligned here (can likely be taken care of
while committing).

Jan

Re: [PATCH v4 5/6] xen/riscv: add section for device information in linker script

[Jan Beulich]

On 26.09.2024 18:54, Oleksii Kurochko wrote:
> Introduce a new `.dev.info` section in the RISC-V linker script to
> handle device-specific information. This section is required by
> common code (common/device.c: device_init(), device_get_class() ).
> This section is aligned to `POINTER_ALIGN`, with `_sdevice` and `_edevice`
> marking the start and end of the section, respectively.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v4 3/6] xen/arm: use {DT,ACPI}_DEV_INFO for device info sections

[Michal Orzel]

Hi Oleksii,

On 26/09/2024 18:54, Oleksii Kurochko wrote:
> 
> 
> Refactor arm/xen.lds.S by replacing the inline definitions for
> device info sections with the newly introduced {DT,ACPI}_DEV_INFO
> macros from xen/xen.lds.h.

I would expect so see a note about s/8/POINTER_ALIGN/ that it's safe to do that.

> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V4:
>  - use newly refactored ACPI_DEV_INFO and DT_DEV_INFO
> ---
> Changes in V3:
>  - use refactored ADEV_INFO and DT_DEV_INFO macros.
> ---
>  xen/arch/arm/xen.lds.S | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 0987052f1a..3b7b677197 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -4,6 +4,8 @@
> 
>  #include <xen/cache.h>
>  #include <xen/lib.h>
> +
> +#define SIMPLE_DECL_SECTION
>  #include <xen/xen.lds.h>
>  #include <asm/page.h>
>  #undef ENTRY
> @@ -124,20 +126,10 @@ SECTIONS
>        _eplatform = .;
>    } :text
> 
> -  . = ALIGN(8);
> -  .dev.info : {
> -      _sdevice = .;
> -      *(.dev.info)
> -      _edevice = .;
> -  } :text
> +  DT_DEV_INFO(.dev.info)
> 
>  #ifdef CONFIG_ACPI
> -  . = ALIGN(8);
> -  .adev.info : {
The name of the section is ".adev.info", but ...
> -      _asdevice = .;
> -      *(.adev.info)
> -      _aedevice = .;
> -  } :text
> +  ACPI_DEV_INFO(adev.info)
here you're missing the leading dot which will cause the probe to fail.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v4 2/6] xen: define ACPI and DT device info sections macros

[Jan Beulich]

On 26.09.2024 18:54, Oleksii Kurochko wrote:
> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -120,6 +120,14 @@
>  
>  /* List of constructs other than *_SECTIONS in alphabetical order. */
>  
> +#define ACPI_DEV_INFO(secname)  \
> +  . = ALIGN(POINTER_ALIGN);     \
> +  DECL_SECTION(secname) {       \
> +      _asdevice = .;            \
> +      *(secname)                \
> +      _aedevice = .;            \
> +  } :text
> +
>  #define BUGFRAMES                               \
>      __start_bug_frames_0 = .;                   \
>      *(.bug_frames.0)                            \
> @@ -137,6 +145,14 @@
>      *(.bug_frames.3)                            \
>      __stop_bug_frames_3 = .;
>  
> +#define DT_DEV_INFO(secname)    \
> +  . = ALIGN(POINTER_ALIGN);     \
> +  DECL_SECTION(secname) {       \
> +       _sdevice = .;           \
> +       *(secname)              \
> +       _edevice = .;           \
> +  } :text
> +
>  #ifdef CONFIG_HYPFS
>  #define HYPFS_PARAM              \
>         . = ALIGN(POINTER_ALIGN); \

Michal's comment made me notice that I overlooked the section names being
macro parameters. Why's that?

Jan

Re: [PATCH v4 6/6] xen/common: move device initialization code to common code

[Michal Orzel]

On 26/09/2024 18:54, Oleksii Kurochko wrote:
> 
> 
> 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.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Roger Pau Monné]

On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
> Introduce SIMPLE_DECL_SECTION to cover the case when
> an architecture wants to declare a section without specifying
> of load address for the section.
> 
> Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.

No strong opinion, but I feel SIMPLE is not very descriptive.  It
might be better to do it the other way around: introduce a define for
when the DECL_SECTION macro should specify a load address:
DECL_SECTION_WITH_LADDR for example.

> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V4:
>  - new patch
> ---
>  xen/arch/x86/xen.lds.S    | 6 ++++--
>  xen/include/xen/xen.lds.h | 6 ++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index b60d2f0d82..9275a566e1 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -3,6 +3,10 @@
>  
>  #include <xen/cache.h>
>  #include <xen/lib.h>
> +
> +#ifdef EFI
> +#define SIMPLE_DECL_SECTION
> +#endif

A nit, but we have been trying to add some indentation to make the
ifdef blocks easier to read, so this would become:

#ifdef EFI
# define SIMPLE_DECL_SECTION
#endif

If it's not too much fuzz to adjust here and below.

Thanks, Roger.

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[oleksii.kurochko]

On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote:
> On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
> > Introduce SIMPLE_DECL_SECTION to cover the case when
> > an architecture wants to declare a section without specifying
> > of load address for the section.
> > 
> > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
> 
> No strong opinion, but I feel SIMPLE is not very descriptive.  It
> might be better to do it the other way around: introduce a define for
> when the DECL_SECTION macro should specify a load address:
> DECL_SECTION_WITH_LADDR for example.
In the next patch, two sections are introduced: dt_dev_info and
acpi_dev_info. The definition of these sections has been made common
and moved to xen.lds.h, and it looks like this:
   +#define DT_DEV_INFO(secname)    \
   +  . = ALIGN(POINTER_ALIGN);     \
   +  DECL_SECTION(secname) {       \
   +       _sdevice = .;           \
   +       *(secname)              \
   +       _edevice = .;           \
   +  } :text
(A similar approach is used for ACPI, please refer to the next patch in
this series.)

For PPC, DECL_SECTION should specify a load address, whereas for Arm
and RISC-V, it should not.

With this generalization, the name of DECL_SECTION should have the same
name in both cases, whether a load address needs to be specified or not

~ Oleksii

Re: [PATCH v4 3/6] xen/arm: use {DT,ACPI}_DEV_INFO for device info sections

[oleksii.kurochko]

Hi Michal,

On Fri, 2024-09-27 at 09:16 +0200, Michal Orzel wrote:
> Hi Oleksii,
> 
> On 26/09/2024 18:54, Oleksii Kurochko wrote:
> > 
> > 
> > Refactor arm/xen.lds.S by replacing the inline definitions for
> > device info sections with the newly introduced {DT,ACPI}_DEV_INFO
> > macros from xen/xen.lds.h.
> 
> I would expect so see a note about s/8/POINTER_ALIGN/ that it's safe
> to do that.
Agree, it would be good to mention, so I will update the commit message
in the next patch version.

> 
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V4:
> >  - use newly refactored ACPI_DEV_INFO and DT_DEV_INFO
> > ---
> > Changes in V3:
> >  - use refactored ADEV_INFO and DT_DEV_INFO macros.
> > ---
> >  xen/arch/arm/xen.lds.S | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 0987052f1a..3b7b677197 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -4,6 +4,8 @@
> > 
> >  #include <xen/cache.h>
> >  #include <xen/lib.h>
> > +
> > +#define SIMPLE_DECL_SECTION
> >  #include <xen/xen.lds.h>
> >  #include <asm/page.h>
> >  #undef ENTRY
> > @@ -124,20 +126,10 @@ SECTIONS
> >        _eplatform = .;
> >    } :text
> > 
> > -  . = ALIGN(8);
> > -  .dev.info : {
> > -      _sdevice = .;
> > -      *(.dev.info)
> > -      _edevice = .;
> > -  } :text
> > +  DT_DEV_INFO(.dev.info)
> > 
> >  #ifdef CONFIG_ACPI
> > -  . = ALIGN(8);
> > -  .adev.info : {
> The name of the section is ".adev.info", but ...
> > -      _asdevice = .;
> > -      *(.adev.info)
> > -      _aedevice = .;
> > -  } :text
> > +  ACPI_DEV_INFO(adev.info)
> here you're missing the leading dot which will cause the probe to
> fail.
Overlooked that, interesting then out CI&CD doesn't check CONFIG_ACPI
for ARM enough... Anyway, I will update that in the next patch version.

> 
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

~ Oleksii

Re: [PATCH v4 2/6] xen: define ACPI and DT device info sections macros

[oleksii.kurochko]

On Fri, 2024-09-27 at 09:28 +0200, Jan Beulich wrote:
> On 26.09.2024 18:54, Oleksii Kurochko wrote:
> > --- a/xen/include/xen/xen.lds.h
> > +++ b/xen/include/xen/xen.lds.h
> > @@ -120,6 +120,14 @@
> >  
> >  /* List of constructs other than *_SECTIONS in alphabetical order.
> > */
> >  
> > +#define ACPI_DEV_INFO(secname)  \
> > +  . = ALIGN(POINTER_ALIGN);     \
> > +  DECL_SECTION(secname) {       \
> > +      _asdevice = .;            \
> > +      *(secname)                \
> > +      _aedevice = .;            \
> > +  } :text
> > +
> >  #define BUGFRAMES                               \
> >      __start_bug_frames_0 = .;                   \
> >      *(.bug_frames.0)                            \
> > @@ -137,6 +145,14 @@
> >      *(.bug_frames.3)                            \
> >      __stop_bug_frames_3 = .;
> >  
> > +#define DT_DEV_INFO(secname)    \
> > +  . = ALIGN(POINTER_ALIGN);     \
> > +  DECL_SECTION(secname) {       \
> > +       _sdevice = .;           \
> > +       *(secname)              \
> > +       _edevice = .;           \
> > +  } :text
> > +
> >  #ifdef CONFIG_HYPFS
> >  #define HYPFS_PARAM              \
> >         . = ALIGN(POINTER_ALIGN); \
> 
> Michal's comment made me notice that I overlooked the section names
> being
> macro parameters. Why's that?
I wanted to give ability for architecture to choose the name but now I
am realized that it isn't corrected as common code is using specific
names ".adev.info" and ".dev.info".

I will prepare the new version of patch series with dropped macro
parameter and just hardcode the name of the section inside the macros.

~ Oleksii

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Roger Pau Monné]

On Fri, Sep 27, 2024 at 11:07:58AM +0200, oleksii.kurochko@gmail.com wrote:
> On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote:
> > On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
> > > Introduce SIMPLE_DECL_SECTION to cover the case when
> > > an architecture wants to declare a section without specifying
> > > of load address for the section.
> > > 
> > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
> > 
> > No strong opinion, but I feel SIMPLE is not very descriptive.  It
> > might be better to do it the other way around: introduce a define for
> > when the DECL_SECTION macro should specify a load address:
> > DECL_SECTION_WITH_LADDR for example.
> In the next patch, two sections are introduced: dt_dev_info and
> acpi_dev_info. The definition of these sections has been made common
> and moved to xen.lds.h, and it looks like this:
>    +#define DT_DEV_INFO(secname)    \
>    +  . = ALIGN(POINTER_ALIGN);     \
>    +  DECL_SECTION(secname) {       \
>    +       _sdevice = .;           \
>    +       *(secname)              \
>    +       _edevice = .;           \
>    +  } :text
> (A similar approach is used for ACPI, please refer to the next patch in
> this series.)
> 
> For PPC, DECL_SECTION should specify a load address, whereas for Arm
> and RISC-V, it should not.
> 
> With this generalization, the name of DECL_SECTION should have the same
> name in both cases, whether a load address needs to be specified or not

Oh, sorry, I think you misunderstood my suggestion.

I'm not suggesting to introduce a new macro named
DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR
instead of SIMPLE_DECL_SECTION in order to signal whether
DECL_SECTION() should specify a load address or not, iow:

#ifdef DECL_SECTION_WITH_LADDR
# define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
#else
# define DECL_SECTION(x) x :
#endif

Thanks, Roger.

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[oleksii.kurochko]

On Fri, 2024-09-27 at 11:41 +0200, Roger Pau Monné wrote:
> On Fri, Sep 27, 2024 at 11:07:58AM +0200,
> oleksii.kurochko@gmail.com wrote:
> > On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote:
> > > On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
> > > > Introduce SIMPLE_DECL_SECTION to cover the case when
> > > > an architecture wants to declare a section without specifying
> > > > of load address for the section.
> > > > 
> > > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
> > > 
> > > No strong opinion, but I feel SIMPLE is not very descriptive.  It
> > > might be better to do it the other way around: introduce a define
> > > for
> > > when the DECL_SECTION macro should specify a load address:
> > > DECL_SECTION_WITH_LADDR for example.
> > In the next patch, two sections are introduced: dt_dev_info and
> > acpi_dev_info. The definition of these sections has been made
> > common
> > and moved to xen.lds.h, and it looks like this:
> >    +#define DT_DEV_INFO(secname)    \
> >    +  . = ALIGN(POINTER_ALIGN);     \
> >    +  DECL_SECTION(secname) {       \
> >    +       _sdevice = .;           \
> >    +       *(secname)              \
> >    +       _edevice = .;           \
> >    +  } :text
> > (A similar approach is used for ACPI, please refer to the next
> > patch in
> > this series.)
> > 
> > For PPC, DECL_SECTION should specify a load address, whereas for
> > Arm
> > and RISC-V, it should not.
> > 
> > With this generalization, the name of DECL_SECTION should have the
> > same
> > name in both cases, whether a load address needs to be specified or
> > not
> 
> Oh, sorry, I think you misunderstood my suggestion.
> 
> I'm not suggesting to introduce a new macro named
> DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR
> instead of SIMPLE_DECL_SECTION in order to signal whether
> DECL_SECTION() should specify a load address or not, iow:
> 
> #ifdef DECL_SECTION_WITH_LADDR
> # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
> #else
> # define DECL_SECTION(x) x :
> #endif
Thanks for the clarification, I really misunderstood your initial
suggestion.

I'm okay with the renaming; perhaps it will indeed make things a bit
clearer.

If Jan doesn’t mind (since he gave the Ack), I'll rename the define in
the next patch version.
Jan, do you mind if I proceed with the renaming?

~ Oleksii

Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Jan Beulich]

On 27.09.2024 12:42, oleksii.kurochko@gmail.com wrote:
> On Fri, 2024-09-27 at 11:41 +0200, Roger Pau Monné wrote:
>> On Fri, Sep 27, 2024 at 11:07:58AM +0200,
>> oleksii.kurochko@gmail.com wrote:
>>> On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote:
>>>> On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
>>>>> Introduce SIMPLE_DECL_SECTION to cover the case when
>>>>> an architecture wants to declare a section without specifying
>>>>> of load address for the section.
>>>>>
>>>>> Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
>>>>
>>>> No strong opinion, but I feel SIMPLE is not very descriptive.  It
>>>> might be better to do it the other way around: introduce a define
>>>> for
>>>> when the DECL_SECTION macro should specify a load address:
>>>> DECL_SECTION_WITH_LADDR for example.
>>> In the next patch, two sections are introduced: dt_dev_info and
>>> acpi_dev_info. The definition of these sections has been made
>>> common
>>> and moved to xen.lds.h, and it looks like this:
>>>    +#define DT_DEV_INFO(secname)    \
>>>    +  . = ALIGN(POINTER_ALIGN);     \
>>>    +  DECL_SECTION(secname) {       \
>>>    +       _sdevice = .;           \
>>>    +       *(secname)              \
>>>    +       _edevice = .;           \
>>>    +  } :text
>>> (A similar approach is used for ACPI, please refer to the next
>>> patch in
>>> this series.)
>>>
>>> For PPC, DECL_SECTION should specify a load address, whereas for
>>> Arm
>>> and RISC-V, it should not.
>>>
>>> With this generalization, the name of DECL_SECTION should have the
>>> same
>>> name in both cases, whether a load address needs to be specified or
>>> not
>>
>> Oh, sorry, I think you misunderstood my suggestion.
>>
>> I'm not suggesting to introduce a new macro named
>> DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR
>> instead of SIMPLE_DECL_SECTION in order to signal whether
>> DECL_SECTION() should specify a load address or not, iow:
>>
>> #ifdef DECL_SECTION_WITH_LADDR
>> # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
>> #else
>> # define DECL_SECTION(x) x :
>> #endif
> Thanks for the clarification, I really misunderstood your initial
> suggestion.
> 
> I'm okay with the renaming; perhaps it will indeed make things a bit
> clearer.
> 
> If Jan doesn’t mind (since he gave the Ack), I'll rename the define in
> the next patch version.
> Jan, do you mind if I proceed with the renaming?

I'm not overly fussed, so fee free to go ahead and retain my ack.

Jan