[PATCH v6 0/9] Introduce generic headers

[PATCH v6 0/9] Introduce generic headers

[Oleksii Kurochko]

Some headers are common between several architectures, so the current patch series
provide them.

Another one reason to have them as generic is a simplification of adding support
necessary to make a complete Xen build as it was/is being done in the patch series [1]
and [2].

Also, instead of providing generic/stub headers, it was used
"#ifdef CONFIG_* #include <asm/*.h> #endif" instead of providing empty headers.

This patch series is a pre-requisite for "Enable build of full Xen for RISC-V" [3].

[1] https://lore.kernel.org/xen-devel/cover.1694543103.git.sanastasio@raptorengineering.com/
[2] https://lore.kernel.org/xen-devel/cover.1692181079.git.oleksii.kurochko@gmail.com/
[3] https://lore.kernel.org/xen-devel/cover.1700761381.git.oleksii.kurochko@gmail.com/

---
Changes in V6:
 - Fix the build script to work properly with EXTRA_FIXED_RANDCONFIG.
 - Introduce separate randconfig yaml with fixed configs for RISC-V.
 - Disable CONFIG_GRANT_TABLE and CONFIG_MEM_ACCESS for PPC and RISC-V.
 - Remove change in Kconfig/common for CONFIG_GRANT_TABLE and CONFIG_MEM_ACCESS.
 - Rebase on top of the latest staging. 
---
Changes in V5:
 - Update the patch series message as patch related to delay.h was merged.
 - Rebase on top of staging because half of the patches of the patch series were
   merged to staging branch.
 - Add A-by for some of the patches.
 - Add "depends on X86 || Arm" for CONFIG_GRANT_TABLE and CONFIG_MEM_ACCESS to be
   sure it won't be turned on by randconfig in CI.
 - Partly switch Arm and PPC to asm-generic/monitor.h.
 - Some other minor changes
---
Changes in V4:
 - Update the cover letter message
 - Add Reviewed-by/Acked-by for patches:
    [PATCH v3 01/14] xen/asm-generic: introduce stub header paging.h
    [PATCH v3 03/14] xen/asm-generic: introduce generic hypercall.h
    [PATCH v3 04/14] xen/asm-generic: introduce generic header iocap.h
    [PATCH v3 05/14] xen/asm-generic: introduce stub header <asm/random.h>
    [PATCH v3 06/14] xen/asm-generic: introduce generic header percpu.h
    [PATCH v3 07/14] xen/asm-generic: introduce generalized hardirq.h
    [PATCH v3 08/14] xen/asm-generic: introduce generic div64.h header
    [PATCH v3 09/14] xen/asm-generic: introduce generic header altp2m.h
    [PATCH v3 10/14] xen/asm-generic: introduce stub header monitor.h
    [PATCH v3 11/14] xen/asm-generic: introduce stub header numa.h
    [PATCH v3 12/14] xen/asm-generic: introduce stub header softirq.h
 - Fix some code style and minor issues.
 - Use asm-generic version of device.h for Arm and PPC.
---
Changes in V3:
 - Update the commit message of the cover letter.
 - Drop the following patch as it can be arch-specific enough:
   * [PATCH v2 09/15] xen/asm-generic: introduce generic header smp.h
 - Drop correspondent arch specific headers and use asm-generic version of
   a header.
 - Back to the patch series patches:
   * xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>
   * xen/asm-generic: ifdef inclusion of <asm/mem_access.h>
---
Changes in V2:
 - Update the commit message of the cover letter.
 - Drop the following patches because they are arch-specific or was sent as a separate patch:
   - xen/asm-generic: introduce stub header event.h
	 - xen/asm-generic: introduce stub header spinlock.h
	 - [PATCH v1 03/29] xen/asm-generic: introduce stub header cpufeature.h
	 - [PATCH v1 07/29] xen/asm-generic: introduce stub header guest_atomics.h
	 - [PATCH v1 10/29] xen/asm-generic: introduce stub header iommu.h
	 - [PATCH v1 12/29] xen/asm-generic: introduce stub header pci.h because separate patch was sent [5]
	 - [PATCH v1 14/29] xen/asm-generic: introduce stub header setup.h
	 - [PATCH v1 15/29] xen/asm-generic: introduce stub header xenoprof.h because of [3].
	 - [PATCH v1 16/29] xen/asm-generic: introduce stub header flushtlb.h
	 - [PATCH v1 22/29] xen/asm-generic: introduce stub header delay.h because of [3]
	 - [PATCH v1 23/29] xen/asm-generic: introduce stub header domain.h
	 - [PATCH v1 24/29] xen/asm-generic: introduce stub header guest_access.h
	 - [PATCH v1 25/29] xen/asm-generic: introduce stub header irq.h ( probably not so generic as I expected, I'll back to it if it will be necessary in the future )
	 - [PATCH v1 28/29] xen/asm-generic: introduce stub header p2m.h ( probably not so generic as I expected, I'll back to it if it will be necessary in the future )
 - For the rest of the patches please look at changes for each patch separately.

 ---

Oleksii Kurochko (9):
  automation: ensure values in EXTRA_FIXED_RANDCONFIG are separated by
    new line
  automation: introduce fixed randconfig for RISC-V
  xen/asm-generic: introduce generic div64.h header
  xen/asm-generic: introduce stub header monitor.h
  xen/asm-generic: introduce stub header numa.h
  xen/asm-generic: introduce stub header softirq.h
  xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>
  xen/asm-generic: ifdef inclusion of <asm/mem_access.h>
  xen/asm-generic: introduce generic device.h

 automation/gitlab-ci/build.yaml               |   8 +-
 .../gitlab-ci/riscv-fixed-randconfig.yaml     |   7 +
 automation/scripts/build                      |   6 +-
 xen/arch/arm/device.c                         |  15 ++-
 xen/arch/arm/domain_build.c                   |   3 +-
 xen/arch/arm/gic-v2.c                         |   4 +-
 xen/arch/arm/gic-v3.c                         |   6 +-
 xen/arch/arm/gic.c                            |   4 +-
 xen/arch/arm/include/asm/Makefile             |   3 +
 xen/arch/arm/include/asm/div64.h              |   8 +-
 xen/arch/arm/include/asm/monitor.h            |  28 +---
 xen/arch/arm/p2m.c                            |   1 +
 xen/arch/arm/traps.c                          |   1 +
 xen/arch/ppc/configs/ppc64_defconfig          |   1 +
 xen/arch/ppc/include/asm/Makefile             |   4 +
 xen/arch/ppc/include/asm/device.h             |  53 --------
 xen/arch/ppc/include/asm/div64.h              |  14 --
 xen/arch/ppc/include/asm/grant_table.h        |   5 -
 xen/arch/ppc/include/asm/mem_access.h         |   5 -
 xen/arch/ppc/include/asm/monitor.h            |  28 +---
 xen/arch/ppc/include/asm/numa.h               |  26 ----
 xen/arch/ppc/include/asm/softirq.h            |   8 --
 xen/arch/riscv/configs/tiny64_defconfig       |   1 +
 xen/arch/x86/include/asm/Makefile             |   1 +
 xen/arch/x86/include/asm/div64.h              |  14 --
 .../asm => include/asm-generic}/device.h      | 125 +++++++++++-------
 xen/include/asm-generic/div64.h               |  27 ++++
 xen/include/asm-generic/monitor.h             |  57 ++++++++
 .../asm => include/asm-generic}/numa.h        |  16 ++-
 .../asm => include/asm-generic}/softirq.h     |   7 +-
 xen/include/xen/grant_table.h                 |   3 +
 xen/include/xen/mem_access.h                  |   2 +
 32 files changed, 234 insertions(+), 257 deletions(-)
 create mode 100644 automation/gitlab-ci/riscv-fixed-randconfig.yaml
 delete mode 100644 xen/arch/ppc/include/asm/device.h
 delete mode 100644 xen/arch/ppc/include/asm/div64.h
 delete mode 100644 xen/arch/ppc/include/asm/grant_table.h
 delete mode 100644 xen/arch/ppc/include/asm/mem_access.h
 delete mode 100644 xen/arch/ppc/include/asm/numa.h
 delete mode 100644 xen/arch/ppc/include/asm/softirq.h
 delete mode 100644 xen/arch/x86/include/asm/div64.h
 rename xen/{arch/arm/include/asm => include/asm-generic}/device.h (79%)
 create mode 100644 xen/include/asm-generic/div64.h
 create mode 100644 xen/include/asm-generic/monitor.h
 rename xen/{arch/arm/include/asm => include/asm-generic}/numa.h (67%)
 rename xen/{arch/arm/include/asm => include/asm-generic}/softirq.h (56%)

-- 
2.43.0

[PATCH v6 1/9] automation: ensure values in EXTRA_FIXED_RANDCONFIG are separated by new line

[Oleksii Kurochko]

Kconfig tool expects each configuration to be on a new line.

The current version of the build script puts all of ${EXTRA_FIXED_RANDCONFIG}
in a single line and configs are seperated by spaces.

As a result, only the first configuration in ${EXTRA_FIXED_RANDCONFIG} will
be used.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
 - The patch was introduced in this version of patch series.
---
 automation/scripts/build | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/automation/scripts/build b/automation/scripts/build
index b3c71fb6fb..13b043923d 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -14,7 +14,7 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
 
     # Append job-specific fixed configuration
     if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then
-        echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config
+        sed "s/ /\n/g" <<< "${EXTRA_FIXED_RANDCONFIG}" > xen/tools/kconfig/allrandom.config
     fi
 
     make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig
@@ -28,9 +28,11 @@ else
     echo "CONFIG_DEBUG=${debug}" >> xen/.config
 
     if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
-        echo "${EXTRA_XEN_CONFIG}" >> xen/.config
+        sed "s/ /\n/g" <<< "${EXTRA_XEN_CONFIG}" >> xen/.config
     fi
 
+    cat xen/.config
+
     make -j$(nproc) -C xen olddefconfig
 fi
 
-- 
2.43.0

Re: [PATCH v6 1/9] automation: ensure values in EXTRA_FIXED_RANDCONFIG are separated by new line

[Oleksii]

Hello Doug abd Stefano,

Could you please take a look at this patch and the next one [1] of this
patch series?

Thanks in advance.

[1]https://lore.kernel.org/xen-devel/db2c3d36b25b686bf07ac23f8ee8c879e0e9e81d.1703072575.git.oleksii.kurochko@gmail.com/

Best regards,
 Oleksii


On Wed, 2023-12-20 at 16:08 +0200, Oleksii Kurochko wrote:
> Kconfig tool expects each configuration to be on a new line.
> 
> The current version of the build script puts all of
> ${EXTRA_FIXED_RANDCONFIG}
> in a single line and configs are seperated by spaces.
> 
> As a result, only the first configuration in
> ${EXTRA_FIXED_RANDCONFIG} will
> be used.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V6:
>  - The patch was introduced in this version of patch series.
> ---
>  automation/scripts/build | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/automation/scripts/build b/automation/scripts/build
> index b3c71fb6fb..13b043923d 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -14,7 +14,7 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>  
>      # Append job-specific fixed configuration
>      if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then
> -        echo "${EXTRA_FIXED_RANDCONFIG}" >>
> xen/tools/kconfig/allrandom.config
> +        sed "s/ /\n/g" <<< "${EXTRA_FIXED_RANDCONFIG}" >
> xen/tools/kconfig/allrandom.config
>      fi
>  
>      make -j$(nproc) -C xen
> KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig
> @@ -28,9 +28,11 @@ else
>      echo "CONFIG_DEBUG=${debug}" >> xen/.config
>  
>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
> -        echo "${EXTRA_XEN_CONFIG}" >> xen/.config
> +        sed "s/ /\n/g" <<< "${EXTRA_XEN_CONFIG}" >> xen/.config
>      fi
>  
> +    cat xen/.config
> +
>      make -j$(nproc) -C xen olddefconfig
>  fi
>  

[PATCH v6 2/9] automation: introduce fixed randconfig for RISC-V

[Oleksii Kurochko]

This patch introduces the file riscv-fixed-randconfig.yaml,
which includes all configurations that should be disabled for
randconfig builds.

Suggested-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
 - The patch was introduced in this version of patch series.
---
 automation/gitlab-ci/build.yaml                  | 8 ++++----
 automation/gitlab-ci/riscv-fixed-randconfig.yaml | 7 +++++++
 2 files changed, 11 insertions(+), 4 deletions(-)
 create mode 100644 automation/gitlab-ci/riscv-fixed-randconfig.yaml

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 32af30cced..38cd93c306 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -516,6 +516,8 @@ alpine-3.18-gcc-debug-arm64-boot-cpupools:
       CONFIG_BOOT_TIME_CPUPOOLS=y
 
 # RISC-V 64 cross-build
+include: 'automation/gitlab-ci/riscv-fixed-randconfig.yaml'
+
 archlinux-current-gcc-riscv64:
   extends: .gcc-riscv64-cross-build
   variables:
@@ -536,8 +538,7 @@ archlinux-current-gcc-riscv64-randconfig:
     CONTAINER: archlinux:current-riscv64
     KBUILD_DEFCONFIG: tiny64_defconfig
     RANDCONFIG: y
-    EXTRA_FIXED_RANDCONFIG:
-      CONFIG_COVERAGE=n
+    EXTRA_FIXED_RANDCONFIG: !reference [.riscv-fixed-randconfig, variables, EXTRA_FIXED_RANDCONFIG]
 
 archlinux-current-gcc-riscv64-debug-randconfig:
   extends: .gcc-riscv64-cross-build-debug
@@ -545,8 +546,7 @@ archlinux-current-gcc-riscv64-debug-randconfig:
     CONTAINER: archlinux:current-riscv64
     KBUILD_DEFCONFIG: tiny64_defconfig
     RANDCONFIG: y
-    EXTRA_FIXED_RANDCONFIG:
-      CONFIG_COVERAGE=n
+    EXTRA_FIXED_RANDCONFIG: !reference [.riscv-fixed-randconfig, variables, EXTRA_FIXED_RANDCONFIG]
 
 # Power cross-build
 debian-bullseye-gcc-ppc64le:
diff --git a/automation/gitlab-ci/riscv-fixed-randconfig.yaml b/automation/gitlab-ci/riscv-fixed-randconfig.yaml
new file mode 100644
index 0000000000..f1282b40c9
--- /dev/null
+++ b/automation/gitlab-ci/riscv-fixed-randconfig.yaml
@@ -0,0 +1,7 @@
+.riscv-fixed-randconfig:
+  variables:
+    EXTRA_FIXED_RANDCONFIG:
+      CONFIG_COVERAGE=n
+      CONFIG_EXPERT=y
+      CONFIG_GRANT_TABLE=n
+      CONFIG_MEM_ACCESS=n
-- 
2.43.0

[PATCH v6 3/9] xen/asm-generic: introduce generic div64.h header

[Oleksii Kurochko]

All archs have the do_div implementation for BITS_PER_LONG == 64
so do_div64.h is moved to asm-generic.

x86 and PPC were switched to asm-generic version of div64.h.
Arm was switched partly because Arm has different implementation
for 32-bits.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - add Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
  - Update the commit message
  - Partly switch Arm's div64.h to asm-generic version. Arm has different
    implementation for 32-bits so only 64-bit version was switched.
---
Changes in V4:
 - Added Acked-by: Jan Beulich <jbeulich@suse.com>.
 - include <asm-generic/div64.h> in Arm's div64.h for 64-bit case.
---
Changes in V3:
 - Drop x86 and PPC's div64.h.
 - Update the commit message.
---
Changes in V2:
	- rename base to divisor
	- add "#if BITS_PER_LONG == 64"
	- fix code style
---
 xen/arch/arm/include/asm/div64.h  |  8 +-------
 xen/arch/ppc/include/asm/Makefile |  1 +
 xen/arch/ppc/include/asm/div64.h  | 14 --------------
 xen/arch/x86/include/asm/Makefile |  1 +
 xen/arch/x86/include/asm/div64.h  | 14 --------------
 xen/include/asm-generic/div64.h   | 27 +++++++++++++++++++++++++++
 6 files changed, 30 insertions(+), 35 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/div64.h
 delete mode 100644 xen/arch/x86/include/asm/div64.h
 create mode 100644 xen/include/asm-generic/div64.h

diff --git a/xen/arch/arm/include/asm/div64.h b/xen/arch/arm/include/asm/div64.h
index fc667a80f9..0459d5cc01 100644
--- a/xen/arch/arm/include/asm/div64.h
+++ b/xen/arch/arm/include/asm/div64.h
@@ -24,13 +24,7 @@
 
 #if BITS_PER_LONG == 64
 
-# define do_div(n,base) ({                                      \
-        uint32_t __base = (base);                               \
-        uint32_t __rem;                                         \
-        __rem = ((uint64_t)(n)) % __base;                       \
-        (n) = ((uint64_t)(n)) / __base;                         \
-        __rem;                                                  \
- })
+#include <asm-generic/div64.h>
 
 #elif BITS_PER_LONG == 32
 
diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
index 2da995bb2f..a8e848d4d0 100644
--- a/xen/arch/ppc/include/asm/Makefile
+++ b/xen/arch/ppc/include/asm/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 generic-y += altp2m.h
+generic-y += div64.h
 generic-y += hardirq.h
 generic-y += hypercall.h
 generic-y += iocap.h
diff --git a/xen/arch/ppc/include/asm/div64.h b/xen/arch/ppc/include/asm/div64.h
deleted file mode 100644
index d213e50585..0000000000
--- a/xen/arch/ppc/include/asm/div64.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-#ifndef __ASM_PPC_DIV64_H__
-#define __ASM_PPC_DIV64_H__
-
-#include <xen/types.h>
-
-#define do_div(n, base) ({                       \
-    uint32_t base_ = (base);                     \
-    uint32_t rem_ = (uint64_t)(n) % base_;       \
-    (n) = (uint64_t)(n) / base_;                 \
-    rem_;                                        \
-})
-
-#endif /* __ASM_PPC_DIV64_H__ */
diff --git a/xen/arch/x86/include/asm/Makefile b/xen/arch/x86/include/asm/Makefile
index 874429ed30..daab34ff0a 100644
--- a/xen/arch/x86/include/asm/Makefile
+++ b/xen/arch/x86/include/asm/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
+generic-y += div64.h
 generic-y += percpu.h
diff --git a/xen/arch/x86/include/asm/div64.h b/xen/arch/x86/include/asm/div64.h
deleted file mode 100644
index dd49f64a3b..0000000000
--- a/xen/arch/x86/include/asm/div64.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef __X86_DIV64
-#define __X86_DIV64
-
-#include <xen/types.h>
-
-#define do_div(n,base) ({                       \
-    uint32_t __base = (base);                   \
-    uint32_t __rem;                             \
-    __rem = ((uint64_t)(n)) % __base;           \
-    (n) = ((uint64_t)(n)) / __base;             \
-    __rem;                                      \
-})
-
-#endif
diff --git a/xen/include/asm-generic/div64.h b/xen/include/asm-generic/div64.h
new file mode 100644
index 0000000000..068d8a11ad
--- /dev/null
+++ b/xen/include/asm-generic/div64.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DIV64
+#define __ASM_GENERIC_DIV64
+
+#include <xen/types.h>
+
+#if BITS_PER_LONG == 64
+
+#define do_div(n, divisor) ({                   \
+    uint32_t divisor_ = (divisor);              \
+    uint32_t rem_ = (uint64_t)(n) % divisor_;   \
+    (n) = (uint64_t)(n) / divisor_;             \
+    rem_;                                       \
+})
+
+#endif /* BITS_PER_LONG */
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH v6 3/9] xen/asm-generic: introduce generic div64.h header

[Julien Grall]

Hi Oleksii,

On 20/12/2023 14:08, Oleksii Kurochko wrote:
> All archs have the do_div implementation for BITS_PER_LONG == 64
> so do_div64.h is moved to asm-generic.
> 
> x86 and PPC were switched to asm-generic version of div64.h.
> Arm was switched partly because Arm has different implementation
> for 32-bits.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Oleksii Kurochko]

The header is shared between several archs so it is
moved to asm-generic.

Switch partly Arm and PPC to asm-generic/monitor.h and only
arch_monitor_get_capabilities() left in arch-specific/monitor.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - Switched partly Arm and PPC to asm-generic monitor.h only
    arch_monitor_get_capabilities() left in arch-specific/monitor.h.
  - Updated the commit message.
---
Changes in V4:
 - Removed the double blank line.
 - Added Acked-by: Jan Beulich <jbeulich@suse.com>.
 - Update the commit message
---
Changes in V3:
 - Use forward-declaration of struct domain instead of " #include <xen/sched.h> ".
 - Add ' include <xen/errno.h> '
 - Drop PPC's monitor.h.
---
Changes in V2:
	- remove inclusion of "+#include <public/domctl.h>"
	- add "struct xen_domctl_monitor_op;"
	- remove one of SPDX tags.
---
 xen/arch/arm/include/asm/monitor.h | 28 +--------------
 xen/arch/ppc/include/asm/monitor.h | 28 +--------------
 xen/include/asm-generic/monitor.h  | 57 ++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 54 deletions(-)
 create mode 100644 xen/include/asm-generic/monitor.h

diff --git a/xen/arch/arm/include/asm/monitor.h b/xen/arch/arm/include/asm/monitor.h
index 7567be66bd..045217c310 100644
--- a/xen/arch/arm/include/asm/monitor.h
+++ b/xen/arch/arm/include/asm/monitor.h
@@ -25,33 +25,7 @@
 #include <xen/sched.h>
 #include <public/domctl.h>
 
-static inline
-void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
-{
-}
-
-static inline
-int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
-{
-    /* No arch-specific monitor ops on ARM. */
-    return -EOPNOTSUPP;
-}
-
-int arch_monitor_domctl_event(struct domain *d,
-                              struct xen_domctl_monitor_op *mop);
-
-static inline
-int arch_monitor_init_domain(struct domain *d)
-{
-    /* No arch-specific domain initialization on ARM. */
-    return 0;
-}
-
-static inline
-void arch_monitor_cleanup_domain(struct domain *d)
-{
-    /* No arch-specific domain cleanup on ARM. */
-}
+#include <asm-generic/monitor.h>
 
 static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
 {
diff --git a/xen/arch/ppc/include/asm/monitor.h b/xen/arch/ppc/include/asm/monitor.h
index e5b0282bf1..89000dacc6 100644
--- a/xen/arch/ppc/include/asm/monitor.h
+++ b/xen/arch/ppc/include/asm/monitor.h
@@ -6,33 +6,7 @@
 #include <public/domctl.h>
 #include <xen/errno.h>
 
-static inline
-void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
-{
-}
-
-static inline
-int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
-{
-    /* No arch-specific monitor ops on PPC. */
-    return -EOPNOTSUPP;
-}
-
-int arch_monitor_domctl_event(struct domain *d,
-                              struct xen_domctl_monitor_op *mop);
-
-static inline
-int arch_monitor_init_domain(struct domain *d)
-{
-    /* No arch-specific domain initialization on PPC. */
-    return 0;
-}
-
-static inline
-void arch_monitor_cleanup_domain(struct domain *d)
-{
-    /* No arch-specific domain cleanup on PPC. */
-}
+#include <asm-generic/monitor.h>
 
 static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
 {
diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
new file mode 100644
index 0000000000..74e4870cd7
--- /dev/null
+++ b/xen/include/asm-generic/monitor.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * include/asm-generic/monitor.h
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ */
+
+#ifndef __ASM_GENERIC_MONITOR_H__
+#define __ASM_GENERIC_MONITOR_H__
+
+#include <xen/errno.h>
+
+struct domain;
+struct xen_domctl_monitor_op;
+
+static inline
+void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
+{
+}
+
+static inline
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+    /* No arch-specific monitor ops on GENERIC. */
+    return -EOPNOTSUPP;
+}
+
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop);
+
+static inline
+int arch_monitor_init_domain(struct domain *d)
+{
+    /* No arch-specific domain initialization on GENERIC. */
+    return 0;
+}
+
+static inline
+void arch_monitor_cleanup_domain(struct domain *d)
+{
+    /* No arch-specific domain cleanup on GENERIC. */
+}
+
+#endif /* __ASM_GENERIC_MONITOR_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: BSD
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Oleksii]

It is necessary to remove unnecessary inclusions of headers in
arch/*/asm/monitor.h. This was overlooked.

Sorry for the inconvenience.

~ Oleksii

On Wed, 2023-12-20 at 16:08 +0200, Oleksii Kurochko wrote:
> The header is shared between several archs so it is
> moved to asm-generic.
> 
> Switch partly Arm and PPC to asm-generic/monitor.h and only
> arch_monitor_get_capabilities() left in arch-specific/monitor.h.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes in V6:
>  - Rebase only.
> ---
> Changes in V5:
>   - Switched partly Arm and PPC to asm-generic monitor.h only
>     arch_monitor_get_capabilities() left in arch-specific/monitor.h.
>   - Updated the commit message.
> ---
> Changes in V4:
>  - Removed the double blank line.
>  - Added Acked-by: Jan Beulich <jbeulich@suse.com>.
>  - Update the commit message
> ---
> Changes in V3:
>  - Use forward-declaration of struct domain instead of " #include
> <xen/sched.h> ".
>  - Add ' include <xen/errno.h> '
>  - Drop PPC's monitor.h.
> ---
> Changes in V2:
> 	- remove inclusion of "+#include <public/domctl.h>"
> 	- add "struct xen_domctl_monitor_op;"
> 	- remove one of SPDX tags.
> ---
>  xen/arch/arm/include/asm/monitor.h | 28 +--------------
>  xen/arch/ppc/include/asm/monitor.h | 28 +--------------
>  xen/include/asm-generic/monitor.h  | 57
> ++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 54 deletions(-)
>  create mode 100644 xen/include/asm-generic/monitor.h
> 
> diff --git a/xen/arch/arm/include/asm/monitor.h
> b/xen/arch/arm/include/asm/monitor.h
> index 7567be66bd..045217c310 100644
> --- a/xen/arch/arm/include/asm/monitor.h
> +++ b/xen/arch/arm/include/asm/monitor.h
> @@ -25,33 +25,7 @@
>  #include <xen/sched.h>
>  #include <public/domctl.h>
>  
> -static inline
> -void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> -{
> -}
> -
> -static inline
> -int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> -{
> -    /* No arch-specific monitor ops on ARM. */
> -    return -EOPNOTSUPP;
> -}
> -
> -int arch_monitor_domctl_event(struct domain *d,
> -                              struct xen_domctl_monitor_op *mop);
> -
> -static inline
> -int arch_monitor_init_domain(struct domain *d)
> -{
> -    /* No arch-specific domain initialization on ARM. */
> -    return 0;
> -}
> -
> -static inline
> -void arch_monitor_cleanup_domain(struct domain *d)
> -{
> -    /* No arch-specific domain cleanup on ARM. */
> -}
> +#include <asm-generic/monitor.h>
>  
>  static inline uint32_t arch_monitor_get_capabilities(struct domain
> *d)
>  {
> diff --git a/xen/arch/ppc/include/asm/monitor.h
> b/xen/arch/ppc/include/asm/monitor.h
> index e5b0282bf1..89000dacc6 100644
> --- a/xen/arch/ppc/include/asm/monitor.h
> +++ b/xen/arch/ppc/include/asm/monitor.h
> @@ -6,33 +6,7 @@
>  #include <public/domctl.h>
>  #include <xen/errno.h>
>  
> -static inline
> -void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> -{
> -}
> -
> -static inline
> -int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> -{
> -    /* No arch-specific monitor ops on PPC. */
> -    return -EOPNOTSUPP;
> -}
> -
> -int arch_monitor_domctl_event(struct domain *d,
> -                              struct xen_domctl_monitor_op *mop);
> -
> -static inline
> -int arch_monitor_init_domain(struct domain *d)
> -{
> -    /* No arch-specific domain initialization on PPC. */
> -    return 0;
> -}
> -
> -static inline
> -void arch_monitor_cleanup_domain(struct domain *d)
> -{
> -    /* No arch-specific domain cleanup on PPC. */
> -}
> +#include <asm-generic/monitor.h>
>  
>  static inline uint32_t arch_monitor_get_capabilities(struct domain
> *d)
>  {
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
> generic/monitor.h
> new file mode 100644
> index 0000000000..74e4870cd7
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/asm-generic/monitor.h
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + */
> +
> +#ifndef __ASM_GENERIC_MONITOR_H__
> +#define __ASM_GENERIC_MONITOR_H__
> +
> +#include <xen/errno.h>
> +
> +struct domain;
> +struct xen_domctl_monitor_op;
> +
> +static inline
> +void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> +{
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> +{
> +    /* No arch-specific monitor ops on GENERIC. */
> +    return -EOPNOTSUPP;
> +}
> +
> +int arch_monitor_domctl_event(struct domain *d,
> +                              struct xen_domctl_monitor_op *mop);
> +
> +static inline
> +int arch_monitor_init_domain(struct domain *d)
> +{
> +    /* No arch-specific domain initialization on GENERIC. */
> +    return 0;
> +}
> +
> +static inline
> +void arch_monitor_cleanup_domain(struct domain *d)
> +{
> +    /* No arch-specific domain cleanup on GENERIC. */
> +}
> +
> +#endif /* __ASM_GENERIC_MONITOR_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: BSD
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Re: [PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Andrew Cooper]

On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
> new file mode 100644
> index 0000000000..74e4870cd7
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/asm-generic/monitor.h
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + */
> +
> +#ifndef __ASM_GENERIC_MONITOR_H__
> +#define __ASM_GENERIC_MONITOR_H__
> +
> +#include <xen/errno.h>
> +
> +struct domain;
> +struct xen_domctl_monitor_op;
> +
> +static inline
> +void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
> +{
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> +    /* No arch-specific monitor ops on GENERIC. */
> +    return -EOPNOTSUPP;
> +}
> +
> +int arch_monitor_domctl_event(struct domain *d,
> +                              struct xen_domctl_monitor_op *mop);

Turn this into a static inline like the others, and you can delete:

arch/ppc/stubs.c:100

int arch_monitor_domctl_event(struct domain *d,
                              struct xen_domctl_monitor_op *mop)
{
    BUG_ON("unimplemented");
}

because new architectures shouldn't have to stub one random piece of a
subsystem when using the generic "nothing special" header.

Given the filtering for arch_monitor_domctl_op(), this one probably
wants to be ASSERT_UNREACHABLE(); return 0.

~Andrew

Re: [PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Oleksii]

On Wed, 2023-12-20 at 16:33 +0000, Andrew Cooper wrote:
> On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
> > generic/monitor.h
> > new file mode 100644
> > index 0000000000..74e4870cd7
> > --- /dev/null
> > +++ b/xen/include/asm-generic/monitor.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * include/asm-generic/monitor.h
> > + *
> > + * Arch-specific monitor_op domctl handler.
> > + *
> > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> > + * Copyright (c) 2016, Bitdefender S.R.L.
> > + *
> > + */
> > +
> > +#ifndef __ASM_GENERIC_MONITOR_H__
> > +#define __ASM_GENERIC_MONITOR_H__
> > +
> > +#include <xen/errno.h>
> > +
> > +struct domain;
> > +struct xen_domctl_monitor_op;
> > +
> > +static inline
> > +void arch_monitor_allow_userspace(struct domain *d, bool
> > allow_userspace)
> > +{
> > +}
> > +
> > +static inline
> > +int arch_monitor_domctl_op(struct domain *d, struct
> > xen_domctl_monitor_op *mop)
> > +{
> > +    /* No arch-specific monitor ops on GENERIC. */
> > +    return -EOPNOTSUPP;
> > +}
> > +
> > +int arch_monitor_domctl_event(struct domain *d,
> > +                              struct xen_domctl_monitor_op *mop);
> 
> Turn this into a static inline like the others, and you can delete:
> 
> arch/ppc/stubs.c:100
> 
> int arch_monitor_domctl_event(struct domain *d,
>                               struct xen_domctl_monitor_op *mop)
> {
>     BUG_ON("unimplemented");
> }
> 
> because new architectures shouldn't have to stub one random piece of
> a
> subsystem when using the generic "nothing special" header.
> 
> Given the filtering for arch_monitor_domctl_op(), this one probably
> wants to be ASSERT_UNREACHABLE(); return 0.
What you wrote makes sense. However, doing it that way may limit the
reuse of other parts of the asm-generic header. It would require
introducing an architecture-specific monitor.h header, which would be
nearly identical.

For instance, at present, the only difference between Arm, PPC, and
RISC-V is arch_monitor_domctl_event(). If this function is implemented
with BUG_ON("unimplemented"), reusing the asm-generic monitor.h header
for Arm (as it is partly done now) becomes challenging.

To address this, I propose wrapping arch_monitor_domctl_event() in
#ifdef:

#ifndef HAS_ARCH_MONITOR_DOMCTL_EVENT
int arch_monitor_domctl_event(struct domain *d,
                              struct xen_domctl_monitor_op *mop)
{
    BUG_ON("unimplemented");
}
#endif

In the architecture-specific monitor.h, you would define
HAS_ARCH_MONITOR_DOMCTL_EVENT and provide the architecture-specific
implementation of the function. For example, in the case of Arm:

#ifndef __ASM_ARM_MONITOR_H__
#define __ASM_ARM_MONITOR_H__

#include <xen/sched.h>
#include <public/domctl.h>

#define HAS_ARCH_MONITOR_DOMCTL_EVENT

#include <asm-generic/monitor.h>

static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
{
    uint32_t capabilities = 0;

    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
                    1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);

    return capabilities;
}

int monitor_smc(void);

#endif /* __ASM_ARM_MONITOR_H__ */

This approach maintains a clean and modular structure, allowing for
architecture-specific variations while reusing the majority of the code
from the generic header.

Does it make sense?

~ Oleksii

Re: [PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Jan Beulich]

On 22.12.2023 14:02, Oleksii wrote:
> On Wed, 2023-12-20 at 16:33 +0000, Andrew Cooper wrote:
>> On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
>>> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
>>> generic/monitor.h
>>> new file mode 100644
>>> index 0000000000..74e4870cd7
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/monitor.h
>>> @@ -0,0 +1,57 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * include/asm-generic/monitor.h
>>> + *
>>> + * Arch-specific monitor_op domctl handler.
>>> + *
>>> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
>>> + * Copyright (c) 2016, Bitdefender S.R.L.
>>> + *
>>> + */
>>> +
>>> +#ifndef __ASM_GENERIC_MONITOR_H__
>>> +#define __ASM_GENERIC_MONITOR_H__
>>> +
>>> +#include <xen/errno.h>
>>> +
>>> +struct domain;
>>> +struct xen_domctl_monitor_op;
>>> +
>>> +static inline
>>> +void arch_monitor_allow_userspace(struct domain *d, bool
>>> allow_userspace)
>>> +{
>>> +}
>>> +
>>> +static inline
>>> +int arch_monitor_domctl_op(struct domain *d, struct
>>> xen_domctl_monitor_op *mop)
>>> +{
>>> +    /* No arch-specific monitor ops on GENERIC. */
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +int arch_monitor_domctl_event(struct domain *d,
>>> +                              struct xen_domctl_monitor_op *mop);
>>
>> Turn this into a static inline like the others, and you can delete:
>>
>> arch/ppc/stubs.c:100
>>
>> int arch_monitor_domctl_event(struct domain *d,
>>                               struct xen_domctl_monitor_op *mop)
>> {
>>     BUG_ON("unimplemented");
>> }
>>
>> because new architectures shouldn't have to stub one random piece of
>> a
>> subsystem when using the generic "nothing special" header.
>>
>> Given the filtering for arch_monitor_domctl_op(), this one probably
>> wants to be ASSERT_UNREACHABLE(); return 0.
> What you wrote makes sense. However, doing it that way may limit the
> reuse of other parts of the asm-generic header. It would require
> introducing an architecture-specific monitor.h header, which would be
> nearly identical.
> 
> For instance, at present, the only difference between Arm, PPC, and
> RISC-V is arch_monitor_domctl_event(). If this function is implemented
> with BUG_ON("unimplemented"), reusing the asm-generic monitor.h header
> for Arm (as it is partly done now) becomes challenging.
> 
> To address this, I propose wrapping arch_monitor_domctl_event() in
> #ifdef:
> 
> #ifndef HAS_ARCH_MONITOR_DOMCTL_EVENT
> int arch_monitor_domctl_event(struct domain *d,
>                               struct xen_domctl_monitor_op *mop)
> {
>     BUG_ON("unimplemented");
> }
> #endif
> 
> In the architecture-specific monitor.h, you would define
> HAS_ARCH_MONITOR_DOMCTL_EVENT and provide the architecture-specific
> implementation of the function. For example, in the case of Arm:
> 
> #ifndef __ASM_ARM_MONITOR_H__
> #define __ASM_ARM_MONITOR_H__
> 
> #include <xen/sched.h>
> #include <public/domctl.h>
> 
> #define HAS_ARCH_MONITOR_DOMCTL_EVENT
> 
> #include <asm-generic/monitor.h>
> 
> static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> {
>     uint32_t capabilities = 0;
> 
>     capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
>                     1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
> 
>     return capabilities;
> }
> 
> int monitor_smc(void);
> 
> #endif /* __ASM_ARM_MONITOR_H__ */
> 
> This approach maintains a clean and modular structure, allowing for
> architecture-specific variations while reusing the majority of the code
> from the generic header.
> 
> Does it make sense?

With the state things are in right now in the tree, perhaps yes. But
as with NUMA and other subsystems: Generally the case of the subsystem
not used should be handled in common code. What's in asm-generic/ is
supposed to be a default implementation when the subsystem _is_ used.
Unlike NUMA, there's no Kconfig control for MONITOR (or VM_EVENT).
Hence why getting this sorted is somewhat more involved here; (ab)using
the asm-generic/ header for the time being is an option, but would then
need properly justifying (imo).

Jan

[PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Oleksii Kurochko]

<asm/numa.h> is common through some archs so it is moved
to asm-generic.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - Added Acked-by: Jan Beulich <jbeulich@suse.com>
  - Updated the comment around first_valid_mfn. ( Arm -> GENERIC )
  - Added Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V4:
 - Updated guards name: *ARCH_GENERIC* -> *ASM_GENERIC*.
 - Moved inclusion of xen/mm-frame.h under "#ifndef CONFIG_NUMA".
 - Added Reviewed-by: Michal Orzel <michal.orzel@amd.com>.
---
Changes in V3:
 - Remove old header inclusion in asm-generic numa.h and include
   <xen/mm-frame.h> and <xen/stdint.h>
 - Drop Arm and PPC's numa.h and use asm-generic version instead.
---
Changes in V2:
	- update the commit message.
	- change u8 to uint8_t.
	- add ifnded CONFIG_NUMA.
---
 xen/arch/arm/include/asm/Makefile             |  1 +
 xen/arch/ppc/include/asm/Makefile             |  1 +
 xen/arch/ppc/include/asm/numa.h               | 26 -------------------
 .../asm => include/asm-generic}/numa.h        | 16 +++++++-----
 4 files changed, 12 insertions(+), 32 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/numa.h
 rename xen/{arch/arm/include/asm => include/asm-generic}/numa.h (67%)

diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile
index 8221429c2c..0c855a798a 100644
--- a/xen/arch/arm/include/asm/Makefile
+++ b/xen/arch/arm/include/asm/Makefile
@@ -2,6 +2,7 @@
 generic-y += altp2m.h
 generic-y += hardirq.h
 generic-y += iocap.h
+generic-y += numa.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += random.h
diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
index a8e848d4d0..f09c5ea8a1 100644
--- a/xen/arch/ppc/include/asm/Makefile
+++ b/xen/arch/ppc/include/asm/Makefile
@@ -4,6 +4,7 @@ generic-y += div64.h
 generic-y += hardirq.h
 generic-y += hypercall.h
 generic-y += iocap.h
+generic-y += numa.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += random.h
diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h
deleted file mode 100644
index 7fdf66c3da..0000000000
--- a/xen/arch/ppc/include/asm/numa.h
+++ /dev/null
@@ -1,26 +0,0 @@
-#ifndef __ASM_PPC_NUMA_H__
-#define __ASM_PPC_NUMA_H__
-
-#include <xen/types.h>
-#include <xen/mm.h>
-
-typedef uint8_t nodeid_t;
-
-/* Fake one node for now. See also node_online_map. */
-#define cpu_to_node(cpu) 0
-#define node_to_cpumask(node)   (cpu_online_map)
-
-/*
- * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
- * is required because the dummy helpers are using it.
- */
-extern mfn_t first_valid_mfn;
-
-/* XXX: implement NUMA support */
-#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
-#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
-#define __node_distance(a, b) (20)
-
-#define arch_want_default_dmazone() (false)
-
-#endif /* __ASM_PPC_NUMA_H__ */
diff --git a/xen/arch/arm/include/asm/numa.h b/xen/include/asm-generic/numa.h
similarity index 67%
rename from xen/arch/arm/include/asm/numa.h
rename to xen/include/asm-generic/numa.h
index e2bee2bd82..7f95a77e89 100644
--- a/xen/arch/arm/include/asm/numa.h
+++ b/xen/include/asm-generic/numa.h
@@ -1,18 +1,21 @@
-#ifndef __ARCH_ARM_NUMA_H
-#define __ARCH_ARM_NUMA_H
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_NUMA_H
+#define __ASM_GENERIC_NUMA_H
 
-#include <xen/mm.h>
+#include <xen/stdint.h>
 
-typedef u8 nodeid_t;
+typedef uint8_t nodeid_t;
 
 #ifndef CONFIG_NUMA
 
+#include <xen/mm-frame.h>
+
 /* Fake one node for now. See also node_online_map. */
 #define cpu_to_node(cpu) 0
 #define node_to_cpumask(node)   (cpu_online_map)
 
 /*
- * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
+ * TODO: make first_valid_mfn static when NUMA is supported on GENERIC, this
  * is required because the dummy helpers are using it.
  */
 extern mfn_t first_valid_mfn;
@@ -26,7 +29,8 @@ extern mfn_t first_valid_mfn;
 
 #define arch_want_default_dmazone() (false)
 
-#endif /* __ARCH_ARM_NUMA_H */
+#endif /* __ASM_GENERIC_NUMA_H */
+
 /*
  * Local variables:
  * mode: C
-- 
2.43.0

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Julien Grall]

Hi Oleksii,

On 20/12/2023 14:08, Oleksii Kurochko wrote:
> <asm/numa.h> is common through some archs so it is moved
> to asm-generic.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

I think this patch will need to be rebased on top of the lastest staging 
as this should clash with 51ffb3311895.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Jan Beulich]

On 21.12.2023 20:09, Julien Grall wrote:
> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>> <asm/numa.h> is common through some archs so it is moved
>> to asm-generic.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> 
> I think this patch will need to be rebased on top of the lastest staging 
> as this should clash with 51ffb3311895.

No, and I'd like to withdraw my ack here. In this case a stub header isn't
the right choice imo - the !NUMA case should be handled in common code. I
would have submitted the patch I have, if only the first_valid_mfn patch
hadn't been committed already (which I now need to re-base over).

Jan

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Oleksii]

On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
> On 21.12.2023 20:09, Julien Grall wrote:
> > On 20/12/2023 14:08, Oleksii Kurochko wrote:
> > > <asm/numa.h> is common through some archs so it is moved
> > > to asm-generic.
> > > 
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> > > Acked-by: Jan Beulich <jbeulich@suse.com>
> > > Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> > 
> > I think this patch will need to be rebased on top of the lastest
> > staging 
> > as this should clash with 51ffb3311895.
> 
> No, and I'd like to withdraw my ack here. In this case a stub header
> isn't
> the right choice imo - the !NUMA case should be handled in common
> code. I
> would have submitted the patch I have, if only the first_valid_mfn
> patch
> hadn't been committed already (which I now need to re-base over).
I haven't seen your patch yet (waiting for it), but I assume that
<asm/numa.h> will still be necessary and remain the same across Arm,
PPC, and RISC-V.

What am I missing?

~ Oleksii

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Jan Beulich]

On 22.12.2023 14:07, Oleksii wrote:
> On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
>> On 21.12.2023 20:09, Julien Grall wrote:
>>> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>>>> <asm/numa.h> is common through some archs so it is moved
>>>> to asm-generic.
>>>>
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>
>>> I think this patch will need to be rebased on top of the lastest
>>> staging 
>>> as this should clash with 51ffb3311895.
>>
>> No, and I'd like to withdraw my ack here. In this case a stub header
>> isn't
>> the right choice imo - the !NUMA case should be handled in common
>> code. I
>> would have submitted the patch I have, if only the first_valid_mfn
>> patch
>> hadn't been committed already (which I now need to re-base over).
> I haven't seen your patch yet (waiting for it), but I assume that
> <asm/numa.h> will still be necessary and remain the same across Arm,
> PPC, and RISC-V.
> 
> What am I missing?

asm/numa.h will be necessary for an arch only if it actually has a way
to have CONFIG_NUMA enabled. Below, for your reference, the patch in
the not-yet-rebased form. As you can see, Arm's (and PPC's) header goes
away. If and when they choose to support NUMA, they may need to regain
one; whether at that point we can sort how an asm-generic/ form of the
header might sensibly look like remains to be seen.

Jan

NUMA: no need for asm/numa.h when !NUMA

There's no point in every architecture carrying the same stubs for the
case when NUMA isn't enabled (or even supported). Move all of that to
xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
inclusion of asm/numa.h dependent upon NUMA=y.

Address the TODO regarding first_valid_mfn by making the variable static
when NUMA=y, thus also addressing a Misrs C:2012 rule 8.4 concern. Drop
the not really applicable "implement NUMA support" comment - in a !NUMA
section this simply makes no sense.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is an alternative proposal to
https://lists.xen.org/archives/html/xen-devel/2023-12/msg01586.html
and its earlier forms.

--- unstable.orig/xen/arch/arm/include/asm/numa.h
+++ /dev/null
@@ -1,37 +0,0 @@
-#ifndef __ARCH_ARM_NUMA_H
-#define __ARCH_ARM_NUMA_H
-
-#include <xen/mm.h>
-
-typedef u8 nodeid_t;
-
-#ifndef CONFIG_NUMA
-
-/* Fake one node for now. See also node_online_map. */
-#define cpu_to_node(cpu) 0
-#define node_to_cpumask(node)   (cpu_online_map)
-
-/*
- * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
- * is required because the dummy helpers are using it.
- */
-extern mfn_t first_valid_mfn;
-
-/* XXX: implement NUMA support */
-#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
-#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
-#define __node_distance(a, b) (20)
-
-#endif
-
-#define arch_want_default_dmazone() (false)
-
-#endif /* __ARCH_ARM_NUMA_H */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
--- unstable.orig/xen/arch/arm/smpboot.c
+++ unstable/xen/arch/arm/smpboot.c
@@ -42,7 +42,7 @@ integer_param("maxcpus", max_cpus);
 /* CPU logical map: map xen cpuid to an MPIDR */
 register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
 
-/* Fake one node for now. See also asm/numa.h */
+/* Fake one node for now. See also xen/numa.h */
 nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
 
 /* Xen stack for bringing up the first CPU. */
--- unstable.orig/xen/arch/ppc/include/asm/numa.h
+++ /dev/null
@@ -1,26 +0,0 @@
-#ifndef __ASM_PPC_NUMA_H__
-#define __ASM_PPC_NUMA_H__
-
-#include <xen/types.h>
-#include <xen/mm.h>
-
-typedef uint8_t nodeid_t;
-
-/* Fake one node for now. See also node_online_map. */
-#define cpu_to_node(cpu) 0
-#define node_to_cpumask(node)   (cpu_online_map)
-
-/*
- * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
- * is required because the dummy helpers are using it.
- */
-extern mfn_t first_valid_mfn;
-
-/* XXX: implement NUMA support */
-#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
-#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
-#define __node_distance(a, b) (20)
-
-#define arch_want_default_dmazone() (false)
-
-#endif /* __ASM_PPC_NUMA_H__ */
--- unstable.orig/xen/common/page_alloc.c
+++ unstable/xen/common/page_alloc.c
@@ -140,7 +140,6 @@
 #include <xen/vm_event.h>
 
 #include <asm/flushtlb.h>
-#include <asm/numa.h>
 #include <asm/page.h>
 
 #include <public/sysctl.h>
@@ -256,10 +255,14 @@ static PAGE_LIST_HEAD(page_broken_list);
  * BOOT-TIME ALLOCATOR
  */
 
+#ifndef CONFIG_NUMA
 /*
  * first_valid_mfn is exported because it is use in ARM specific NUMA
  * helpers. See comment in arch/arm/include/asm/numa.h.
  */
+#else
+static
+#endif
 mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
 
 struct bootmem_region {
--- unstable.orig/xen/common/sysctl.c
+++ unstable/xen/common/sysctl.c
@@ -22,8 +22,8 @@
 #include <asm/current.h>
 #include <xen/hypercall.h>
 #include <public/sysctl.h>
-#include <asm/numa.h>
 #include <xen/nodemask.h>
+#include <xen/numa.h>
 #include <xsm/xsm.h>
 #include <xen/pmstat.h>
 #include <xen/livepatch.h>
--- unstable.orig/xen/include/xen/domain.h
+++ unstable/xen/include/xen/domain.h
@@ -2,6 +2,7 @@
 #ifndef __XEN_DOMAIN_H__
 #define __XEN_DOMAIN_H__
 
+#include <xen/numa.h>
 #include <xen/types.h>
 
 #include <public/xen.h>
@@ -12,7 +13,6 @@ struct guest_area {
 };
 
 #include <asm/domain.h>
-#include <asm/numa.h>
 
 typedef union {
     struct vcpu_guest_context *nat;
--- unstable.orig/xen/include/xen/numa.h
+++ unstable/xen/include/xen/numa.h
@@ -2,7 +2,13 @@
 #define _XEN_NUMA_H
 
 #include <xen/mm-frame.h>
+
+#ifdef CONFIG_NUMA
+#include <xen/pdx.h>
 #include <asm/numa.h>
+#else
+typedef uint8_t nodeid_t;
+#endif
 
 #define NUMA_NO_NODE     0xFF
 #define NUMA_NO_DISTANCE 0xFF
@@ -108,6 +114,18 @@ extern void numa_set_processor_nodes_par
 
 #else
 
+/* Fake one node for now. See also node_online_map. */
+#define cpu_to_node(cpu) 0
+#define node_to_cpumask(node)   cpu_online_map
+
+#define arch_want_default_dmazone() false
+
+extern mfn_t first_valid_mfn;
+
+#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
+#define node_start_pfn(nid) mfn_x(first_valid_mfn)
+#define __node_distance(a, b) 20
+
 static inline nodeid_t mfn_to_nid(mfn_t mfn)
 {
     return 0;
--- unstable.orig/xen/include/xen/pci.h
+++ unstable/xen/include/xen/pci.h
@@ -11,10 +11,10 @@
 #include <xen/list.h>
 #include <xen/spinlock.h>
 #include <xen/irq.h>
+#include <xen/numa.h>
 #include <xen/pci_regs.h>
 #include <xen/pfn.h>
 #include <asm/device.h>
-#include <asm/numa.h>
 
 /*
  * The PCI interface treats multi-function devices as independent

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Julien Grall]

Hi Jan,

On 22/12/2023 13:20, Jan Beulich wrote:
> On 22.12.2023 14:07, Oleksii wrote:
>> On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
>>> On 21.12.2023 20:09, Julien Grall wrote:
>>>> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>>>>> <asm/numa.h> is common through some archs so it is moved
>>>>> to asm-generic.
>>>>>
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>>
>>>> I think this patch will need to be rebased on top of the lastest
>>>> staging
>>>> as this should clash with 51ffb3311895.
>>>
>>> No, and I'd like to withdraw my ack here. In this case a stub header
>>> isn't
>>> the right choice imo - the !NUMA case should be handled in common
>>> code. I
>>> would have submitted the patch I have, if only the first_valid_mfn
>>> patch
>>> hadn't been committed already (which I now need to re-base over).
>> I haven't seen your patch yet (waiting for it), but I assume that
>> <asm/numa.h> will still be necessary and remain the same across Arm,
>> PPC, and RISC-V.
>>
>> What am I missing?
> 
> asm/numa.h will be necessary for an arch only if it actually has a way
> to have CONFIG_NUMA enabled. Below, for your reference, the patch in
> the not-yet-rebased form. As you can see, Arm's (and PPC's) header goes
> away. If and when they choose to support NUMA, they may need to regain
> one; whether at that point we can sort how an asm-generic/ form of the
> header might sensibly look like remains to be seen.
> 
> Jan
> 
> NUMA: no need for asm/numa.h when !NUMA
> 
> There's no point in every architecture carrying the same stubs for the
> case when NUMA isn't enabled (or even supported). Move all of that to
> xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
> inclusion of asm/numa.h dependent upon NUMA=y.
> 
> Address the TODO regarding first_valid_mfn by making the variable static
> when NUMA=y, thus also addressing a Misrs C:2012 rule 8.4 concern. Drop
> the not really applicable "implement NUMA support" comment - in a !NUMA
> section this simply makes no sense.

I have to admit, I am not really thrill with the approach you have 
taken. As I said before, I don't quite understand the problem of having 
first_valid_mfn exposed in all the circumstances.

This is better than trying to do...

> --- unstable.orig/xen/common/page_alloc.c
> +++ unstable/xen/common/page_alloc.c
> @@ -140,7 +140,6 @@
>   #include <xen/vm_event.h>
>   
>   #include <asm/flushtlb.h>
> -#include <asm/numa.h>
>   #include <asm/page.h>
>   
>   #include <public/sysctl.h>
> @@ -256,10 +255,14 @@ static PAGE_LIST_HEAD(page_broken_list);
>    * BOOT-TIME ALLOCATOR
>    */
>   
> +#ifndef CONFIG_NUMA
>   /*
>    * first_valid_mfn is exported because it is use in ARM specific NUMA
>    * helpers. See comment in arch/arm/include/asm/numa.h.
>    */
> +#else
> +static
> +#endif

... this sort ugliness.

I am not going to Nack the patch. But I am definitely not going to ack 
it. with this change.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Julien Grall]

On 22/12/2023 13:58, Julien Grall wrote:
> Hi Jan,
> 
> On 22/12/2023 13:20, Jan Beulich wrote:
>> On 22.12.2023 14:07, Oleksii wrote:
>>> On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
>>>> On 21.12.2023 20:09, Julien Grall wrote:
>>>>> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>>>>>> <asm/numa.h> is common through some archs so it is moved
>>>>>> to asm-generic.
>>>>>>
>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>>>
>>>>> I think this patch will need to be rebased on top of the lastest
>>>>> staging
>>>>> as this should clash with 51ffb3311895.
>>>>
>>>> No, and I'd like to withdraw my ack here. In this case a stub header
>>>> isn't
>>>> the right choice imo - the !NUMA case should be handled in common
>>>> code. I
>>>> would have submitted the patch I have, if only the first_valid_mfn
>>>> patch
>>>> hadn't been committed already (which I now need to re-base over).
>>> I haven't seen your patch yet (waiting for it), but I assume that
>>> <asm/numa.h> will still be necessary and remain the same across Arm,
>>> PPC, and RISC-V.
>>>
>>> What am I missing?
>>
>> asm/numa.h will be necessary for an arch only if it actually has a way
>> to have CONFIG_NUMA enabled. Below, for your reference, the patch in
>> the not-yet-rebased form. As you can see, Arm's (and PPC's) header goes
>> away. If and when they choose to support NUMA, they may need to regain
>> one; whether at that point we can sort how an asm-generic/ form of the
>> header might sensibly look like remains to be seen.
>>
>> Jan
>>
>> NUMA: no need for asm/numa.h when !NUMA
>>
>> There's no point in every architecture carrying the same stubs for the
>> case when NUMA isn't enabled (or even supported). Move all of that to
>> xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
>> inclusion of asm/numa.h dependent upon NUMA=y.
>>
>> Address the TODO regarding first_valid_mfn by making the variable static
>> when NUMA=y, thus also addressing a Misrs C:2012 rule 8.4 concern. Drop
>> the not really applicable "implement NUMA support" comment - in a !NUMA
>> section this simply makes no sense.
> 
> I have to admit, I am not really thrill with the approach you have 
> taken. As I said before, I don't quite understand the problem of having 
> first_valid_mfn exposed in all the circumstances.
> 
> This is better than trying to do...
> 
>> --- unstable.orig/xen/common/page_alloc.c
>> +++ unstable/xen/common/page_alloc.c
>> @@ -140,7 +140,6 @@
>>   #include <xen/vm_event.h>
>>   #include <asm/flushtlb.h>
>> -#include <asm/numa.h>
>>   #include <asm/page.h>
>>   #include <public/sysctl.h>
>> @@ -256,10 +255,14 @@ static PAGE_LIST_HEAD(page_broken_list);
>>    * BOOT-TIME ALLOCATOR
>>    */
>> +#ifndef CONFIG_NUMA
>>   /*
>>    * first_valid_mfn is exported because it is use in ARM specific NUMA
>>    * helpers. See comment in arch/arm/include/asm/numa.h.
>>    */
>> +#else
>> +static
>> +#endif
> 
> ... this sort ugliness.

Just to add more thoughts. My main concern here is that we likely have 
other variables that may only be exposed for a given arch. I would 
rather not like if we start sprinkling the code with #ifdef CONFIG_XXX 
static #else.

If there is a desire for everyone to start reducing the scope of the 
variable. Then we should come up with a way to do what you did above on 
the same line rather than multiple one. Maybe:

STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn.

Cheers,

-- 
Julien Grall

Xen 4.19 release schedule proposal

[Oleksii]

Dear community,

Wishing you a Happy New Year!

I'd like to propose the release schedule for Xen 4.19.

Based on the previous release schedules [1] and [2], it seems the next
release date should be on Wednesday, July 10, 2024:

** Proposed option: Wed Jul 10, 2024 **
(+9 months from Xen 4.18 release)

- Last posting date          Fri Apr 26, 2024

Patches adding new features are expected to be posted to the mailing
list by this date, although perhaps not in their final version.

- Feature freeze             Fri May 17, 2024 (+3 weeks from Last
posting date)

Patches adding new features should be committed by this date.
Straightforward bugfixes may continue to be accepted by maintainers.

- Code freeze                Fri May 31, 2024 (+2 weeks from Feature
freeze)

Bugfixes only.

- Hard code freeze           Fri Jun 21, 2024 (+3 weeks from Code
freeze)

Bugfixes for serious bugs (including regressions), and low-risk fixes
only.

- Final commits              Fri Jul 05, 2024 (+2 weeks from Hard code
freeze)

Branch off staging-4.19.

- Release                    Wed Jul 10, 2024

If there are no objections, we will stick to the proposed schedule.

One more thing I'd like to discuss is whether to add a file
(RELEASE.md) with the release schedule to the source code or update an
existing one (xen-release-management.pandoc). I think it will help to
find the final release schedule for the nearest release. For example,
for the previous release, there are a lot of emails with proposed
schedules, polls of Xen release schedules, and I found the final
release schedule in just one of the replies (but probably I missed
something).

I'll be happy to hear any comments from you. Thanks in advance.

Best regards,
 Oleksii

[1]
https://lore.kernel.org/xen-devel/AS8PR08MB7991424A3167C70A9B29530C92929@AS8PR08MB7991.eurprd08.prod.outlook.com/
[2] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html

Re: Xen 4.19 release schedule proposal

[Oleksii]

Something went wrong, and this email wasn't sent as a separate topic,
so I re-sent it.

Sorry for any inconvenience.

~ Oleksii
On Tue, 2024-01-02 at 18:59 +0200, Oleksii wrote:
> Dear community,
> 
> Wishing you a Happy New Year!
> 
> I'd like to propose the release schedule for Xen 4.19.
> 
> Based on the previous release schedules [1] and [2], it seems the
> next
> release date should be on Wednesday, July 10, 2024:
> 
> ** Proposed option: Wed Jul 10, 2024 **
> (+9 months from Xen 4.18 release)
> 
> - Last posting date          Fri Apr 26, 2024
> 
> Patches adding new features are expected to be posted to the mailing
> list by this date, although perhaps not in their final version.
> 
> - Feature freeze             Fri May 17, 2024 (+3 weeks from Last
> posting date)
> 
> Patches adding new features should be committed by this date.
> Straightforward bugfixes may continue to be accepted by maintainers.
> 
> - Code freeze                Fri May 31, 2024 (+2 weeks from Feature
> freeze)
> 
> Bugfixes only.
> 
> - Hard code freeze           Fri Jun 21, 2024 (+3 weeks from Code
> freeze)
> 
> Bugfixes for serious bugs (including regressions), and low-risk fixes
> only.
> 
> - Final commits              Fri Jul 05, 2024 (+2 weeks from Hard
> code
> freeze)
> 
> Branch off staging-4.19.
> 
> - Release                    Wed Jul 10, 2024
> 
> If there are no objections, we will stick to the proposed schedule.
> 
> One more thing I'd like to discuss is whether to add a file
> (RELEASE.md) with the release schedule to the source code or update
> an
> existing one (xen-release-management.pandoc). I think it will help to
> find the final release schedule for the nearest release. For example,
> for the previous release, there are a lot of emails with proposed
> schedules, polls of Xen release schedules, and I found the final
> release schedule in just one of the replies (but probably I missed
> something).
> 
> I'll be happy to hear any comments from you. Thanks in advance.
> 
> Best regards,
>  Oleksii
> 
> [1]
> https://lore.kernel.org/xen-devel/AS8PR08MB7991424A3167C70A9B29530C92929@AS8PR08MB7991.eurprd08.prod.outlook.com/
> [2]
> https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html

Re: Xen 4.19 release schedule proposal

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2069 bytes --]

On 02.01.24 17:59, Oleksii wrote:
> Dear community,
> 
> Wishing you a Happy New Year!
> 
> I'd like to propose the release schedule for Xen 4.19.
> 
> Based on the previous release schedules [1] and [2], it seems the next
> release date should be on Wednesday, July 10, 2024:
> 
> ** Proposed option: Wed Jul 10, 2024 **
> (+9 months from Xen 4.18 release)
> 
> - Last posting date          Fri Apr 26, 2024
> 
> Patches adding new features are expected to be posted to the mailing
> list by this date, although perhaps not in their final version.
> 
> - Feature freeze             Fri May 17, 2024 (+3 weeks from Last
> posting date)
> 
> Patches adding new features should be committed by this date.
> Straightforward bugfixes may continue to be accepted by maintainers.
> 
> - Code freeze                Fri May 31, 2024 (+2 weeks from Feature
> freeze)
> 
> Bugfixes only.
> 
> - Hard code freeze           Fri Jun 21, 2024 (+3 weeks from Code
> freeze)
> 
> Bugfixes for serious bugs (including regressions), and low-risk fixes
> only.
> 
> - Final commits              Fri Jul 05, 2024 (+2 weeks from Hard code
> freeze)
> 
> Branch off staging-4.19.
> 
> - Release                    Wed Jul 10, 2024
> 
> If there are no objections, we will stick to the proposed schedule.
> 
> One more thing I'd like to discuss is whether to add a file
> (RELEASE.md) with the release schedule to the source code or update an
> existing one (xen-release-management.pandoc). I think it will help to
> find the final release schedule for the nearest release. For example,
> for the previous release, there are a lot of emails with proposed
> schedules, polls of Xen release schedules, and I found the final
> release schedule in just one of the replies (but probably I missed
> something).

What about putting it into the wiki under Xen_Project_X.YY_Release_Notes?
That way it would already be accessible via SUPPORT.md (in the wiki under
https://xenbits.xen.org/docs/unstable-staging/support-matrix.html ).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: Xen 4.19 release schedule proposal

[Oleksii]

On Wed, 2024-01-03 at 06:53 +0100, Juergen Gross wrote:
> On 02.01.24 17:59, Oleksii wrote:
> > Dear community,
> > 
> > Wishing you a Happy New Year!
> > 
> > I'd like to propose the release schedule for Xen 4.19.
> > 
> > Based on the previous release schedules [1] and [2], it seems the
> > next
> > release date should be on Wednesday, July 10, 2024:
> > 
> > ** Proposed option: Wed Jul 10, 2024 **
> > (+9 months from Xen 4.18 release)
> > 
> > - Last posting date          Fri Apr 26, 2024
> > 
> > Patches adding new features are expected to be posted to the
> > mailing
> > list by this date, although perhaps not in their final version.
> > 
> > - Feature freeze             Fri May 17, 2024 (+3 weeks from Last
> > posting date)
> > 
> > Patches adding new features should be committed by this date.
> > Straightforward bugfixes may continue to be accepted by
> > maintainers.
> > 
> > - Code freeze                Fri May 31, 2024 (+2 weeks from
> > Feature
> > freeze)
> > 
> > Bugfixes only.
> > 
> > - Hard code freeze           Fri Jun 21, 2024 (+3 weeks from Code
> > freeze)
> > 
> > Bugfixes for serious bugs (including regressions), and low-risk
> > fixes
> > only.
> > 
> > - Final commits              Fri Jul 05, 2024 (+2 weeks from Hard
> > code
> > freeze)
> > 
> > Branch off staging-4.19.
> > 
> > - Release                    Wed Jul 10, 2024
> > 
> > If there are no objections, we will stick to the proposed schedule.
> > 
> > One more thing I'd like to discuss is whether to add a file
> > (RELEASE.md) with the release schedule to the source code or update
> > an
> > existing one (xen-release-management.pandoc). I think it will help
> > to
> > find the final release schedule for the nearest release. For
> > example,
> > for the previous release, there are a lot of emails with proposed
> > schedules, polls of Xen release schedules, and I found the final
> > release schedule in just one of the replies (but probably I missed
> > something).
> 
> What about putting it into the wiki under
> Xen_Project_X.YY_Release_Notes?
> That way it would already be accessible via SUPPORT.md (in the wiki
> under
> https://xenbits.xen.org/docs/unstable-staging/support-matrix.html ).
It makes sense to me. Thanks.

~ Oleksii

Re: Xen 4.19 release schedule proposal

[Jan Beulich]

On 02.01.2024 17:59, Oleksii wrote:
> I'd like to propose the release schedule for Xen 4.19.
> 
> Based on the previous release schedules [1] and [2], it seems the next
> release date should be on Wednesday, July 10, 2024:
> 
> ** Proposed option: Wed Jul 10, 2024 **
> (+9 months from Xen 4.18 release)

Hmm, aren't we intending to be on a 8 month cadence?

Jan

Re: Xen 4.19 release schedule proposal

[Oleksii]

On Thu, 2024-01-04 at 13:52 +0100, Jan Beulich wrote:
> On 02.01.2024 17:59, Oleksii wrote:
> > I'd like to propose the release schedule for Xen 4.19.
> > 
> > Based on the previous release schedules [1] and [2], it seems the
> > next
> > release date should be on Wednesday, July 10, 2024:
> > 
> > ** Proposed option: Wed Jul 10, 2024 **
> > (+9 months from Xen 4.18 release)
> 
> Hmm, aren't we intending to be on a 8 month cadence?
Considering that in July, there will be the Xen Developer Summit, we
can aim for an 8-month cadence.

However, in the Xen release document, there was mention of a discussion
[1] about cadence:
"With 18 months of full support and 36 months of security support, the
number of concurrent supported releases will be the same with either 8
or 9 months release cycles, so I have chosen an 8-month cycle for now."

I interpreted this as either an 8 or 9-month cycle, and it's not
strict. If there's a strict requirement for a specific duration, I'll
resend the Release Schedule Proposal.

[1] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html


~ Oleksii

Re: Xen 4.19 release schedule proposal

[Jan Beulich]

On 08.01.2024 15:37, Oleksii wrote:
> On Thu, 2024-01-04 at 13:52 +0100, Jan Beulich wrote:
>> On 02.01.2024 17:59, Oleksii wrote:
>>> I'd like to propose the release schedule for Xen 4.19.
>>>
>>> Based on the previous release schedules [1] and [2], it seems the
>>> next
>>> release date should be on Wednesday, July 10, 2024:
>>>
>>> ** Proposed option: Wed Jul 10, 2024 **
>>> (+9 months from Xen 4.18 release)
>>
>> Hmm, aren't we intending to be on a 8 month cadence?
> Considering that in July, there will be the Xen Developer Summit, we
> can aim for an 8-month cadence.

July? Iirc I read June in the announcement.

> However, in the Xen release document, there was mention of a discussion
> [1] about cadence:
> "With 18 months of full support and 36 months of security support, the
> number of concurrent supported releases will be the same with either 8
> or 9 months release cycles, so I have chosen an 8-month cycle for now."
> 
> I interpreted this as either an 8 or 9-month cycle, and it's not
> strict. If there's a strict requirement for a specific duration, I'll
> resend the Release Schedule Proposal.

I'm not sure about "strict". Yet ...

> [1] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html

... this very mail worked out how overlap with larger holiday ranges
could be minimized not only for a single release, but for any as long
as the cadence is followed. Iirc this works out better with 8 months
(as kind of to be expected, as then there are only 3 variants, whereas
with 9 months it would be 4 of them).

Just to clarify, personally I'm fine with 9 months or even longer, but
it seemed to me that we had settled on 8.

Jan

Re: Xen 4.19 release schedule proposal

[Kelly Choi]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hi all,

I propose we raise this in the next community call (Thursday 1st Feb).
We can decide on the official cadence then and any further feedback.

Many thanks,
Kelly Choi

Community Manager
Xen Project


On Mon, Jan 8, 2024 at 2:58 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 08.01.2024 15:37, Oleksii wrote:
> > On Thu, 2024-01-04 at 13:52 +0100, Jan Beulich wrote:
> >> On 02.01.2024 17:59, Oleksii wrote:
> >>> I'd like to propose the release schedule for Xen 4.19.
> >>>
> >>> Based on the previous release schedules [1] and [2], it seems the
> >>> next
> >>> release date should be on Wednesday, July 10, 2024:
> >>>
> >>> ** Proposed option: Wed Jul 10, 2024 **
> >>> (+9 months from Xen 4.18 release)
> >>
> >> Hmm, aren't we intending to be on a 8 month cadence?
> > Considering that in July, there will be the Xen Developer Summit, we
> > can aim for an 8-month cadence.
>
> July? Iirc I read June in the announcement.
>
> > However, in the Xen release document, there was mention of a discussion
> > [1] about cadence:
> > "With 18 months of full support and 36 months of security support, the
> > number of concurrent supported releases will be the same with either 8
> > or 9 months release cycles, so I have chosen an 8-month cycle for now."
> >
> > I interpreted this as either an 8 or 9-month cycle, and it's not
> > strict. If there's a strict requirement for a specific duration, I'll
> > resend the Release Schedule Proposal.
>
> I'm not sure about "strict". Yet ...
>
> > [1] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html
>
> ... this very mail worked out how overlap with larger holiday ranges
> could be minimized not only for a single release, but for any as long
> as the cadence is followed. Iirc this works out better with 8 months
> (as kind of to be expected, as then there are only 3 variants, whereas
> with 9 months it would be 4 of them).
>
> Just to clarify, personally I'm fine with 9 months or even longer, but
> it seemed to me that we had settled on 8.
>
> Jan
>

[-- Attachment #2: Type: text/html, Size: 3039 bytes --]

Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Jan Beulich]

On 20.12.2023 15:08, Oleksii Kurochko wrote:
> <asm/numa.h> is common through some archs so it is moved
> to asm-generic.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

I'd like to withdraw my ack here. As said elsewhere (and see the
respective patch), I don't think a generic header is wanted or
needed here (nor in similar other cases). !NUMA logic ought to live
in xen/numa.h.

Jan

> ---
> Changes in V6:
>  - Rebase only.
> ---
> Changes in V5:
>   - Added Acked-by: Jan Beulich <jbeulich@suse.com>
>   - Updated the comment around first_valid_mfn. ( Arm -> GENERIC )
>   - Added Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
> Changes in V4:
>  - Updated guards name: *ARCH_GENERIC* -> *ASM_GENERIC*.
>  - Moved inclusion of xen/mm-frame.h under "#ifndef CONFIG_NUMA".
>  - Added Reviewed-by: Michal Orzel <michal.orzel@amd.com>.
> ---
> Changes in V3:
>  - Remove old header inclusion in asm-generic numa.h and include
>    <xen/mm-frame.h> and <xen/stdint.h>
>  - Drop Arm and PPC's numa.h and use asm-generic version instead.
> ---
> Changes in V2:
> 	- update the commit message.
> 	- change u8 to uint8_t.
> 	- add ifnded CONFIG_NUMA.
> ---
>  xen/arch/arm/include/asm/Makefile             |  1 +
>  xen/arch/ppc/include/asm/Makefile             |  1 +
>  xen/arch/ppc/include/asm/numa.h               | 26 -------------------
>  .../asm => include/asm-generic}/numa.h        | 16 +++++++-----
>  4 files changed, 12 insertions(+), 32 deletions(-)
>  delete mode 100644 xen/arch/ppc/include/asm/numa.h
>  rename xen/{arch/arm/include/asm => include/asm-generic}/numa.h (67%)
> 
> diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile
> index 8221429c2c..0c855a798a 100644
> --- a/xen/arch/arm/include/asm/Makefile
> +++ b/xen/arch/arm/include/asm/Makefile
> @@ -2,6 +2,7 @@
>  generic-y += altp2m.h
>  generic-y += hardirq.h
>  generic-y += iocap.h
> +generic-y += numa.h
>  generic-y += paging.h
>  generic-y += percpu.h
>  generic-y += random.h
> diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
> index a8e848d4d0..f09c5ea8a1 100644
> --- a/xen/arch/ppc/include/asm/Makefile
> +++ b/xen/arch/ppc/include/asm/Makefile
> @@ -4,6 +4,7 @@ generic-y += div64.h
>  generic-y += hardirq.h
>  generic-y += hypercall.h
>  generic-y += iocap.h
> +generic-y += numa.h
>  generic-y += paging.h
>  generic-y += percpu.h
>  generic-y += random.h
> diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h
> deleted file mode 100644
> index 7fdf66c3da..0000000000
> --- a/xen/arch/ppc/include/asm/numa.h
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -#ifndef __ASM_PPC_NUMA_H__
> -#define __ASM_PPC_NUMA_H__
> -
> -#include <xen/types.h>
> -#include <xen/mm.h>
> -
> -typedef uint8_t nodeid_t;
> -
> -/* Fake one node for now. See also node_online_map. */
> -#define cpu_to_node(cpu) 0
> -#define node_to_cpumask(node)   (cpu_online_map)
> -
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
> -/* XXX: implement NUMA support */
> -#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> -#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> -#define __node_distance(a, b) (20)
> -
> -#define arch_want_default_dmazone() (false)
> -
> -#endif /* __ASM_PPC_NUMA_H__ */
> diff --git a/xen/arch/arm/include/asm/numa.h b/xen/include/asm-generic/numa.h
> similarity index 67%
> rename from xen/arch/arm/include/asm/numa.h
> rename to xen/include/asm-generic/numa.h
> index e2bee2bd82..7f95a77e89 100644
> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/include/asm-generic/numa.h
> @@ -1,18 +1,21 @@
> -#ifndef __ARCH_ARM_NUMA_H
> -#define __ARCH_ARM_NUMA_H
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_NUMA_H
> +#define __ASM_GENERIC_NUMA_H
>  
> -#include <xen/mm.h>
> +#include <xen/stdint.h>
>  
> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
>  
>  #ifndef CONFIG_NUMA
>  
> +#include <xen/mm-frame.h>
> +
>  /* Fake one node for now. See also node_online_map. */
>  #define cpu_to_node(cpu) 0
>  #define node_to_cpumask(node)   (cpu_online_map)
>  
>  /*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> + * TODO: make first_valid_mfn static when NUMA is supported on GENERIC, this
>   * is required because the dummy helpers are using it.
>   */
>  extern mfn_t first_valid_mfn;
> @@ -26,7 +29,8 @@ extern mfn_t first_valid_mfn;
>  
>  #define arch_want_default_dmazone() (false)
>  
> -#endif /* __ARCH_ARM_NUMA_H */
> +#endif /* __ASM_GENERIC_NUMA_H */
> +
>  /*
>   * Local variables:
>   * mode: C

[PATCH v6 6/9] xen/asm-generic: introduce stub header softirq.h

[Oleksii Kurochko]

<asm/softirq.h> is common between Arm, PPC and RISC-V so it is
moved to asm-generic.

Drop Arm and PPC's softirq.h and use asm-generic version instead.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
 - Strayed "Added" in commit message
 - Added Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in V4:
 - Added Reviewed-by: Michal Orzel <michal.orzel@amd.com>
 - Added Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V3:
 - Drop Arm and PPC's softirq.h
 - Update the commit message.
---
Changes in V2:
    - update the commit message.
---
 xen/arch/arm/include/asm/Makefile                         | 1 +
 xen/arch/ppc/include/asm/Makefile                         | 1 +
 xen/arch/ppc/include/asm/softirq.h                        | 8 --------
 .../arm/include/asm => include/asm-generic}/softirq.h     | 7 ++++---
 4 files changed, 6 insertions(+), 11 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/softirq.h
 rename xen/{arch/arm/include/asm => include/asm-generic}/softirq.h (56%)

diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile
index 0c855a798a..a28cc5d1b1 100644
--- a/xen/arch/arm/include/asm/Makefile
+++ b/xen/arch/arm/include/asm/Makefile
@@ -6,4 +6,5 @@ generic-y += numa.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += random.h
+generic-y += softirq.h
 generic-y += vm_event.h
diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
index f09c5ea8a1..efd72862c8 100644
--- a/xen/arch/ppc/include/asm/Makefile
+++ b/xen/arch/ppc/include/asm/Makefile
@@ -8,4 +8,5 @@ generic-y += numa.h
 generic-y += paging.h
 generic-y += percpu.h
 generic-y += random.h
+generic-y += softirq.h
 generic-y += vm_event.h
diff --git a/xen/arch/ppc/include/asm/softirq.h b/xen/arch/ppc/include/asm/softirq.h
deleted file mode 100644
index a0b28a5e51..0000000000
--- a/xen/arch/ppc/include/asm/softirq.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef __ASM_PPC_SOFTIRQ_H__
-#define __ASM_PPC_SOFTIRQ_H__
-
-#define NR_ARCH_SOFTIRQS 0
-
-#define arch_skip_send_event_check(cpu) 0
-
-#endif /* __ASM_PPC_SOFTIRQ_H__ */
diff --git a/xen/arch/arm/include/asm/softirq.h b/xen/include/asm-generic/softirq.h
similarity index 56%
rename from xen/arch/arm/include/asm/softirq.h
rename to xen/include/asm-generic/softirq.h
index 976e0ebd70..83be855e50 100644
--- a/xen/arch/arm/include/asm/softirq.h
+++ b/xen/include/asm-generic/softirq.h
@@ -1,11 +1,12 @@
-#ifndef __ASM_SOFTIRQ_H__
-#define __ASM_SOFTIRQ_H__
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_SOFTIRQ_H__
+#define __ASM_GENERIC_SOFTIRQ_H__
 
 #define NR_ARCH_SOFTIRQS       0
 
 #define arch_skip_send_event_check(cpu) 0
 
-#endif /* __ASM_SOFTIRQ_H__ */
+#endif /* __ASM_GENERIC_SOFTIRQ_H__ */
 /*
  * Local variables:
  * mode: C
-- 
2.43.0

Re: [PATCH v6 6/9] xen/asm-generic: introduce stub header softirq.h

[Julien Grall]

Hi Oksii,

On 20/12/2023 14:08, Oleksii Kurochko wrote:
> <asm/softirq.h> is common between Arm, PPC and RISC-V so it is
> moved to asm-generic.
> 
> Drop Arm and PPC's softirq.h and use asm-generic version instead.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v6 7/9] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Oleksii Kurochko]

Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid
generation of empty <asm/grant_table.h> for cases when
CONFIG_GRANT_TABLE is not enabled.

The following changes were done for Arm:
<asm/grant_table.h> should be included directly because it contains
gnttab_dom0_frames() macros which is unique for Arm and is used in
arch/arm/domain_build.c.
<asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in
<xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames
won't be available for use in arch/arm/domain_build.c.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
 - Remove the way how CONFIG_GRANT_TABLE is disabled for PPC and RISC-V.
---
Changes in V5:
 - Added dependencies for "Config GRANT_TABLE" to be sure that randconfig will not
   turn on the config.
---
Changes in V4:
 - Nothing changed. Only rebase.
---
Changes in V3:
 - Remove unnecessary comment.
---
 xen/arch/arm/domain_build.c            | 1 +
 xen/arch/ppc/include/asm/grant_table.h | 5 -----
 xen/include/xen/grant_table.h          | 3 +++
 3 files changed, 4 insertions(+), 5 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/grant_table.h

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6945b9755d..46161848dc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -36,6 +36,7 @@
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
+#include <asm/grant_table.h>
 #include <xen/serial.h>
 
 static unsigned int __initdata opt_dom0_max_vcpus;
diff --git a/xen/arch/ppc/include/asm/grant_table.h b/xen/arch/ppc/include/asm/grant_table.h
deleted file mode 100644
index d0ff58dd3d..0000000000
--- a/xen/arch/ppc/include/asm/grant_table.h
+++ /dev/null
@@ -1,5 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-#ifndef __ASM_PPC_GRANT_TABLE_H__
-#define __ASM_PPC_GRANT_TABLE_H__
-
-#endif /* __ASM_PPC_GRANT_TABLE_H__ */
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 85fe6b7b5e..50edfecfb6 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -26,7 +26,10 @@
 #include <xen/mm-frame.h>
 #include <xen/rwlock.h>
 #include <public/grant_table.h>
+
+#ifdef CONFIG_GRANT_TABLE
 #include <asm/grant_table.h>
+#endif
 
 struct grant_table;
 
-- 
2.43.0

Re: [PATCH v6 7/9] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Julien Grall]

Hi Oleksii,

On 20/12/2023 14:08, Oleksii Kurochko wrote:
> Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid
> generation of empty <asm/grant_table.h> for cases when
> CONFIG_GRANT_TABLE is not enabled.

It would have been nice to explain the reason of this change. Is this a 
compilation error or just a nice thing to have?

The reason I am asking is...

> 
> The following changes were done for Arm:
> <asm/grant_table.h> should be included directly because it contains
> gnttab_dom0_frames() macros which is unique for Arm and is used in
> arch/arm/domain_build.c.
> <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in
> <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames
> won't be available for use in arch/arm/domain_build.c.

... I find rather ugly that we require domain_build.c to include both 
asm/grant_table.h and xen/grant_table.h.

Right now, I don't have a better approach, so I would be ok so long the 
rationale of the change is explained in the commit message.

> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v6 7/9] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Julien Grall]

On 21/12/2023 19:19, Julien Grall wrote:
> Hi Oleksii,
> 
> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>> Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid
>> generation of empty <asm/grant_table.h> for cases when
>> CONFIG_GRANT_TABLE is not enabled.
> 
> It would have been nice to explain the reason of this change. Is this a 
> compilation error or just a nice thing to have?
> 
> The reason I am asking is...
> 
>>
>> The following changes were done for Arm:
>> <asm/grant_table.h> should be included directly because it contains
>> gnttab_dom0_frames() macros which is unique for Arm and is used in
>> arch/arm/domain_build.c.
>> <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in
>> <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames
>> won't be available for use in arch/arm/domain_build.c.
> 
> ... I find rather ugly that we require domain_build.c to include both 
> asm/grant_table.h and xen/grant_table.h.
> 
> Right now, I don't have a better approach, so I would be ok so long the 
> rationale of the change is explained in the commit message.

Urgh, I just realized that this is explained in the commit message. 
Please ignore my comment about expanding the commit message. Sorry for 
the noise.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 7/9] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Oleksii]

Hi Julien,

On Thu, 2023-12-21 at 19:20 +0000, Julien Grall wrote:
> 
> 
> On 21/12/2023 19:19, Julien Grall wrote:
> > Hi Oleksii,
> > 
> > On 20/12/2023 14:08, Oleksii Kurochko wrote:
> > > Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid
> > > generation of empty <asm/grant_table.h> for cases when
> > > CONFIG_GRANT_TABLE is not enabled.
> > 
> > It would have been nice to explain the reason of this change. Is
> > this a 
> > compilation error or just a nice thing to have?
> > 
> > The reason I am asking is...
> > 
> > > 
> > > The following changes were done for Arm:
> > > <asm/grant_table.h> should be included directly because it
> > > contains
> > > gnttab_dom0_frames() macros which is unique for Arm and is used
> > > in
> > > arch/arm/domain_build.c.
> > > <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in
> > > <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE
> > > gnttab_dom0_frames
> > > won't be available for use in arch/arm/domain_build.c.
> > 
> > ... I find rather ugly that we require domain_build.c to include
> > both 
> > asm/grant_table.h and xen/grant_table.h.
> > 
> > Right now, I don't have a better approach, so I would be ok so long
> > the 
> > rationale of the change is explained in the commit message.
> 
> Urgh, I just realized that this is explained in the commit message. 
> Please ignore my comment about expanding the commit message. Sorry
> for 
> the noise.
It's OK.
Thanks for review!

~ Oleksii

Re: [PATCH v6 7/9] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Shawn Anastasio]

Hi Oleksii,

On 12/20/23 8:08 AM, Oleksii Kurochko wrote:
> Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid
> generation of empty <asm/grant_table.h> for cases when
> CONFIG_GRANT_TABLE is not enabled.
> 
> The following changes were done for Arm:
> <asm/grant_table.h> should be included directly because it contains
> gnttab_dom0_frames() macros which is unique for Arm and is used in
> arch/arm/domain_build.c.
> <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in
> <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames
> won't be available for use in arch/arm/domain_build.c.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Thanks,
Shawn

[PATCH v6 8/9] xen/asm-generic: ifdef inclusion of <asm/mem_access.h>

[Oleksii Kurochko]

ifdefing inclusion of <asm/mem_access.h> in <xen/mem_access.h>
allows to avoid generation of empty <asm/mem_access.h> header
for the case when !CONFIG_MEM_ACCESS.

For Arm it was explicitly added inclusion of <asm/mem_access.h> for p2m.c
and traps.c because they require some functions from <asm/mem_access.h> which
aren't available in case of !CONFIG_MEM_ACCESS.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
 - Remove the way how CONFIG_MEM_ACCESS is disabled for PPC and RISC-V.
 - Disable the config in ppc64_defconfig and tiny64_defconfig (RISC-V).
---
Changes in V5:
 - Added dependencies for "Config MEM_ACCESS" to be sure that randconfig will not
   turn on the config.
---
Changes in V4:
 - Nothing changed. Only rebase.
---
Changes in V3:
 - Remove unnecessary comment.
---
 xen/arch/arm/p2m.c                      | 1 +
 xen/arch/arm/traps.c                    | 1 +
 xen/arch/ppc/configs/ppc64_defconfig    | 1 +
 xen/arch/ppc/include/asm/mem_access.h   | 5 -----
 xen/arch/riscv/configs/tiny64_defconfig | 1 +
 xen/include/xen/mem_access.h            | 2 ++
 6 files changed, 6 insertions(+), 5 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/mem_access.h

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b991b76ce4..2465c266e9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -7,6 +7,7 @@
 #include <asm/event.h>
 #include <asm/flushtlb.h>
 #include <asm/guest_walk.h>
+#include <asm/mem_access.h>
 #include <asm/page.h>
 #include <asm/traps.h>
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3784e8276e..37a457f4b1 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -35,6 +35,7 @@
 #include <asm/cpufeature.h>
 #include <asm/event.h>
 #include <asm/hsr.h>
+#include <asm/mem_access.h>
 #include <asm/mmio.h>
 #include <asm/regs.h>
 #include <asm/smccc.h>
diff --git a/xen/arch/ppc/configs/ppc64_defconfig b/xen/arch/ppc/configs/ppc64_defconfig
index f7cc075e45..48a053237a 100644
--- a/xen/arch/ppc/configs/ppc64_defconfig
+++ b/xen/arch/ppc/configs/ppc64_defconfig
@@ -6,6 +6,7 @@
 # CONFIG_HYPFS is not set
 # CONFIG_GRANT_TABLE is not set
 # CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
+# CONFIG_MEM_ACCESS is not set
 
 CONFIG_PPC64=y
 CONFIG_DEBUG=y
diff --git a/xen/arch/ppc/include/asm/mem_access.h b/xen/arch/ppc/include/asm/mem_access.h
deleted file mode 100644
index e7986dfdbd..0000000000
--- a/xen/arch/ppc/include/asm/mem_access.h
+++ /dev/null
@@ -1,5 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-#ifndef __ASM_PPC_MEM_ACCESS_H__
-#define __ASM_PPC_MEM_ACCESS_H__
-
-#endif /* __ASM_PPC_MEM_ACCESS_H__ */
diff --git a/xen/arch/riscv/configs/tiny64_defconfig b/xen/arch/riscv/configs/tiny64_defconfig
index 3c9a2ff941..09defe236b 100644
--- a/xen/arch/riscv/configs/tiny64_defconfig
+++ b/xen/arch/riscv/configs/tiny64_defconfig
@@ -6,6 +6,7 @@
 # CONFIG_HYPFS is not set
 # CONFIG_GRANT_TABLE is not set
 # CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
+# CONFIG_MEM_ACCESS is not set
 
 CONFIG_RISCV_64=y
 CONFIG_DEBUG=y
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 4e4811680d..87d93b31f6 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -33,7 +33,9 @@
  */
 struct vm_event_st;
 
+#ifdef CONFIG_MEM_ACCESS
 #include <asm/mem_access.h>
+#endif
 
 /*
  * Additional access types, which are used to further restrict
-- 
2.43.0

Re: [PATCH v6 8/9] xen/asm-generic: ifdef inclusion of <asm/mem_access.h>

[Shawn Anastasio]

Hi Oleksii,

On 12/20/23 8:08 AM, Oleksii Kurochko wrote:
> ifdefing inclusion of <asm/mem_access.h> in <xen/mem_access.h>
> allows to avoid generation of empty <asm/mem_access.h> header
> for the case when !CONFIG_MEM_ACCESS.
> 
> For Arm it was explicitly added inclusion of <asm/mem_access.h> for p2m.c
> and traps.c because they require some functions from <asm/mem_access.h> which
> aren't available in case of !CONFIG_MEM_ACCESS.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Thanks,
Shawn

[PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Oleksii Kurochko]

Arm, PPC and RISC-V use the same device.h thereby device.h
was moved to asm-generic. Arm's device.h was taken as a base with
the following changes:
 - #ifdef PCI related things.
 - #ifdef ACPI related things.
 - Rename #ifdef guards.
 - Add SPDX tag.
 - #ifdef CONFIG_HAS_DEVICE_TREE related things.
 - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.

Also Arm and PPC are switched to asm-generic version of device.h

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---

     Jan wrote the following:
       Overall I think there are too many changes done all in one go here.
       But it's mostly Arm which is affected, so I'll leave judging about that
       to the Arm maintainers.
     
     Arm maintainers, will it be fine for you to not split the patch?
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - Removed generated file: xen/include/headers++.chk.new
  - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
    CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
 - Updated the commit message
 - Switched Arm and PPC to asm-generic version of device.h
 - Replaced HAS_PCI with CONFIG_HAS_PCI
 - ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
 - Updated the commit message ( remove a note with question about
   if device.h should be in asm-generic or not )
 - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
 - Rationalized usage of CONFIG_HAS_* in device.h
 - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
Changes in V3:
 - ifdef device tree related things.
 - update the commit message
---
Changes in V2:
	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
	- #ifdef PCI related things.
	- #ifdef ACPI related things.
	- rename DEVICE_GIC to DEVIC_IC.
	- rename #ifdef guards.
	- switch Arm and PPC to generic device.h
	- add SPDX tag
	- update the commit message

---
 xen/arch/arm/device.c                         |  15 ++-
 xen/arch/arm/domain_build.c                   |   2 +-
 xen/arch/arm/gic-v2.c                         |   4 +-
 xen/arch/arm/gic-v3.c                         |   6 +-
 xen/arch/arm/gic.c                            |   4 +-
 xen/arch/arm/include/asm/Makefile             |   1 +
 xen/arch/ppc/include/asm/Makefile             |   1 +
 xen/arch/ppc/include/asm/device.h             |  53 --------
 .../asm => include/asm-generic}/device.h      | 125 +++++++++++-------
 9 files changed, 102 insertions(+), 109 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/device.h
 rename xen/{arch/arm/include/asm => include/asm-generic}/device.h (79%)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 1f631d3274..affbe79f9a 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -16,7 +16,10 @@
 #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)
@@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
     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;
@@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class class, const void *data, int class
 
     return -EBADF;
 }
+#endif
 
 enum device_class device_get_class(const struct dt_device_node *dev)
 {
@@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
     struct map_range_data mr_data = {
         .d = d,
         .p2mt = p2mt,
-        .skip_mapping = !own_device ||
-                        (is_pci_passthrough_enabled() &&
-                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+        .skip_mapping =
+                        !own_device
+#ifdef CONFIG_HAS_PCI
+                        || (is_pci_passthrough_enabled() &&
+                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))
+#endif
+                        ,
         .iomem_ranges = iomem_ranges,
         .irq_ranges = irq_ranges
     };
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 46161848dc..085d88671e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1651,7 +1651,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
      * Replace these nodes with our own. Note that the original may be
      * used_by DOMID_XEN so this check comes first.
      */
-    if ( device_get_class(node) == DEVICE_GIC )
+    if ( device_get_class(node) == DEVICE_INTERRUPT_CONTROLLER )
         return make_gic_node(d, kinfo->fdt, node);
     if ( dt_match_node(timer_matches, node) )
         return make_timer_node(kinfo);
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cf392bfd1c..5d6885e389 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1366,7 +1366,7 @@ static const struct dt_device_match gicv2_dt_match[] __initconst =
     { /* sentinel */ },
 };
 
-DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
+DT_DEVICE_START(gicv2, "GICv2", DEVICE_INTERRUPT_CONTROLLER)
         .dt_match = gicv2_dt_match,
         .init = gicv2_dt_preinit,
 DT_DEVICE_END
@@ -1381,7 +1381,7 @@ static int __init gicv2_acpi_preinit(const void *data)
     return 0;
 }
 
-ACPI_DEVICE_START(agicv2, "GICv2", DEVICE_GIC)
+ACPI_DEVICE_START(agicv2, "GICv2", DEVICE_INTERRUPT_CONTROLLER)
         .class_type = ACPI_MADT_GIC_VERSION_V2,
         .init = gicv2_acpi_preinit,
 ACPI_DEVICE_END
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 18289cd645..9e135aeb3d 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1879,7 +1879,7 @@ static const struct dt_device_match gicv3_dt_match[] __initconst =
     { /* sentinel */ },
 };
 
-DT_DEVICE_START(gicv3, "GICv3", DEVICE_GIC)
+DT_DEVICE_START(gicv3, "GICv3", DEVICE_INTERRUPT_CONTROLLER)
         .dt_match = gicv3_dt_match,
         .init = gicv3_dt_preinit,
 DT_DEVICE_END
@@ -1894,12 +1894,12 @@ static int __init gicv3_acpi_preinit(const void *data)
     return 0;
 }
 
-ACPI_DEVICE_START(agicv3, "GICv3", DEVICE_GIC)
+ACPI_DEVICE_START(agicv3, "GICv3", DEVICE_INTERRUPT_CONTROLLER)
         .class_type = ACPI_MADT_GIC_VERSION_V3,
         .init = gicv3_acpi_preinit,
 ACPI_DEVICE_END
 
-ACPI_DEVICE_START(agicv4, "GICv4", DEVICE_GIC)
+ACPI_DEVICE_START(agicv4, "GICv4", DEVICE_INTERRUPT_CONTROLLER)
         .class_type = ACPI_MADT_GIC_VERSION_V4,
         .init = gicv3_acpi_preinit,
 ACPI_DEVICE_END
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d922ea67aa..b5a9c8266c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -234,7 +234,7 @@ static void __init gic_dt_preinit(void)
         if ( !dt_get_parent(node) )
             continue;
 
-        rc = device_init(node, DEVICE_GIC, NULL);
+        rc = device_init(node, DEVICE_INTERRUPT_CONTROLLER, NULL);
         if ( !rc )
         {
             /* NOTE: Only one GIC is supported */
@@ -262,7 +262,7 @@ static void __init gic_acpi_preinit(void)
 
     dist = container_of(header, struct acpi_madt_generic_distributor, header);
 
-    if ( acpi_device_init(DEVICE_GIC, NULL, dist->version) )
+    if ( acpi_device_init(DEVICE_INTERRUPT_CONTROLLER, NULL, dist->version) )
         panic("Unable to find compatible GIC in the ACPI table\n");
 }
 #else
diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile
index a28cc5d1b1..c3f4291ee2 100644
--- a/xen/arch/arm/include/asm/Makefile
+++ b/xen/arch/arm/include/asm/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 generic-y += altp2m.h
+generic-y += device.h
 generic-y += hardirq.h
 generic-y += iocap.h
 generic-y += numa.h
diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
index efd72862c8..adb752b804 100644
--- a/xen/arch/ppc/include/asm/Makefile
+++ b/xen/arch/ppc/include/asm/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 generic-y += altp2m.h
+generic-y += device.h
 generic-y += div64.h
 generic-y += hardirq.h
 generic-y += hypercall.h
diff --git a/xen/arch/ppc/include/asm/device.h b/xen/arch/ppc/include/asm/device.h
deleted file mode 100644
index 8253e61d51..0000000000
--- a/xen/arch/ppc/include/asm/device.h
+++ /dev/null
@@ -1,53 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-#ifndef __ASM_PPC_DEVICE_H__
-#define __ASM_PPC_DEVICE_H__
-
-enum device_type
-{
-    DEV_DT,
-    DEV_PCI,
-};
-
-struct device {
-    enum device_type type;
-#ifdef CONFIG_HAS_DEVICE_TREE
-    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
-#endif
-};
-
-enum device_class
-{
-    DEVICE_SERIAL,
-    DEVICE_IOMMU,
-    DEVICE_PCI_HOSTBRIDGE,
-    /* Use for error */
-    DEVICE_UNKNOWN,
-};
-
-struct device_desc {
-    /* Device name */
-    const char *name;
-    /* Device class */
-    enum device_class class;
-    /* List of devices supported by this driver */
-    const struct dt_device_match *dt_match;
-    /*
-     * Device initialization.
-     *
-     * -EAGAIN is used to indicate that device probing is deferred.
-     */
-    int (*init)(struct dt_device_node *dev, const void *data);
-};
-
-typedef struct device device_t;
-
-#define DT_DEVICE_START(name_, namestr_, class_)                    \
-static const struct device_desc __dev_desc_##name_ __used           \
-__section(".dev.info") = {                                          \
-    .name = namestr_,                                               \
-    .class = class_,                                                \
-
-#define DT_DEVICE_END                                               \
-};
-
-#endif /* __ASM_PPC_DEVICE_H__ */
diff --git a/xen/arch/arm/include/asm/device.h b/xen/include/asm-generic/device.h
similarity index 79%
rename from xen/arch/arm/include/asm/device.h
rename to xen/include/asm-generic/device.h
index b5d451e087..063c448c4e 100644
--- a/xen/arch/arm/include/asm/device.h
+++ b/xen/include/asm-generic/device.h
@@ -1,14 +1,37 @@
-#ifndef __ASM_ARM_DEVICE_H
-#define __ASM_ARM_DEVICE_H
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+#include <xen/stdbool.h>
 
 enum device_type
 {
+#ifdef CONFIG_HAS_DEVICE_TREE
     DEV_DT,
+#endif
+
+#ifdef CONFIG_HAS_PCI
     DEV_PCI,
+#endif
+    DEV_TYPE_MAX,
+};
+
+enum device_class
+{
+    DEVICE_SERIAL,
+    DEVICE_IOMMU,
+    DEVICE_INTERRUPT_CONTROLLER,
+#ifdef CONFIG_HAS_PCI
+    DEVICE_PCI_HOSTBRIDGE,
+#endif
+    /* Use for error */
+    DEVICE_UNKNOWN,
 };
 
 struct dev_archdata {
+#ifdef CONFIG_HAS_PASSTHROUGH
     void *iommu;    /* IOMMU private data */
+#endif
 };
 
 /* struct device - The basic device structure */
@@ -19,31 +42,65 @@ struct device
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
     struct dev_archdata archdata;
+#ifdef CONFIG_HAS_PASSTHROUGH
     struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
+#endif
 };
 
 typedef struct device device_t;
 
+#ifdef CONFIG_HAS_DEVICE_TREE
+
 #include <xen/device_tree.h>
 
-#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
 #define dev_is_dt(dev)  ((dev)->type == DEV_DT)
 
-enum device_class
-{
-    DEVICE_SERIAL,
-    DEVICE_IOMMU,
-    DEVICE_GIC,
-    DEVICE_PCI_HOSTBRIDGE,
-    /* Use for error */
-    DEVICE_UNKNOWN,
+/**
+ *  device_init - Initialize a device
+ *  @dev: device to initialize
+ *  @class: class of the device (serial, network...)
+ *  @data: specific data for initializing the device
+ *
+ *  Return 0 on success.
+ */
+int device_init(struct dt_device_node *dev, enum device_class class,
+                const void *data);
+
+/**
+ * device_get_type - Get the type of the device
+ * @dev: device to match
+ *
+ * Return the device type on success or DEVICE_ANY on failure
+ */
+enum device_class device_get_class(const struct dt_device_node *dev);
+
+#define DT_DEVICE_START(_name, _namestr, _class)                    \
+static const struct device_desc __dev_desc_##_name __used           \
+__section(".dev.info") = {                                          \
+    .name = _namestr,                                               \
+    .class = _class,                                                \
+
+#define DT_DEVICE_END                                               \
 };
 
+#else /* !CONFIG_HAS_DEVICE_TREE */
+#define dev_is_dt(dev) ((void)(dev), false)
+#endif /* CONFIG_HAS_DEVICE_TREE */
+
+#ifdef CONFIG_HAS_PCI
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
+#else
+#define dev_is_pci(dev) ((void)(dev), false)
+#endif
+
 struct device_desc {
     /* Device name */
     const char *name;
     /* Device class */
     enum device_class class;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+
     /* List of devices supported by this driver */
     const struct dt_device_match *dt_match;
     /*
@@ -52,8 +109,12 @@ struct device_desc {
      * -EAGAIN is used to indicate that device probing is deferred.
      */
     int (*init)(struct dt_device_node *dev, const void *data);
+
+#endif
 };
 
+#ifdef CONFIG_ACPI
+
 struct acpi_device_desc {
     /* Device name */
     const char *name;
@@ -75,44 +136,18 @@ struct acpi_device_desc {
 int acpi_device_init(enum device_class class,
                      const void *data, int class_type);
 
-/**
- *  device_init - Initialize a device
- *  @dev: device to initialize
- *  @class: class of the device (serial, network...)
- *  @data: specific data for initializing the device
- *
- *  Return 0 on success.
- */
-int device_init(struct dt_device_node *dev, enum device_class class,
-                const void *data);
-
-/**
- * device_get_type - Get the type of the device
- * @dev: device to match
- *
- * Return the device type on success or DEVICE_ANY on failure
- */
-enum device_class device_get_class(const struct dt_device_node *dev);
+#define ACPI_DEVICE_START(_name, _namestr, _class)              \
+static const struct acpi_device_desc __dev_desc_##_name __used  \
+__section(".adev.info") = {                                     \
+    .name = _namestr,                                           \
+    .class = _class,                                            \
 
-#define DT_DEVICE_START(_name, _namestr, _class)                    \
-static const struct device_desc __dev_desc_##_name __used           \
-__section(".dev.info") = {                                          \
-    .name = _namestr,                                               \
-    .class = _class,                                                \
-
-#define DT_DEVICE_END                                               \
+#define ACPI_DEVICE_END                                         \
 };
 
-#define ACPI_DEVICE_START(_name, _namestr, _class)                    \
-static const struct acpi_device_desc __dev_desc_##_name __used           \
-__section(".adev.info") = {                       \
-    .name = _namestr,                                               \
-    .class = _class,                                                \
-
-#define ACPI_DEVICE_END                                               \
-};
+#endif /* CONFIG_ACPI */
 
-#endif /* __ASM_ARM_DEVICE_H */
+#endif /* __ASM_GENERIC_DEVICE_H__ */
 
 /*
  * Local variables:
-- 
2.43.0

Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Julien Grall]

Hi,

On 20/12/2023 14:08, Oleksii Kurochko wrote:
> Arm, PPC and RISC-V use the same device.h thereby device.h
> was moved to asm-generic. Arm's device.h was taken as a base with
> the following changes:
>   - #ifdef PCI related things.
>   - #ifdef ACPI related things.
>   - Rename #ifdef guards.
>   - Add SPDX tag.
>   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
>   - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
> 
> Also Arm and PPC are switched to asm-generic version of device.h
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> 
>       Jan wrote the following:
>         Overall I think there are too many changes done all in one go here.
>         But it's mostly Arm which is affected, so I'll leave judging about that
>         to the Arm maintainers.
>       
>       Arm maintainers, will it be fine for you to not split the patch?

So in general I agree with Jan, patches should be kept small so they are 
easy to review.

Given the discussion has been on-going for a while (we are at v6),  I 
will give an attempt to review the patch as-is. But in the future, 
please try to split. The smaller it is, the easier to review. For code 
movement and refactoring, I tend to first have a few refactoring patches 
and then move the code in a separate patch. So it is easier to spot the 
differences.

> ---
> Changes in V6:
>   - Rebase only.
> ---
> Changes in V5:
>    - Removed generated file: xen/include/headers++.chk.new
>    - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
>      CONFIG_HAS_DEVICE_TREE will be always used for PPC.
> ---
> Changes in V4:
>   - Updated the commit message
>   - Switched Arm and PPC to asm-generic version of device.h
>   - Replaced HAS_PCI with CONFIG_HAS_PCI
>   - ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
>   - ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
>   - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
>   - Updated the commit message ( remove a note with question about
>     if device.h should be in asm-generic or not )
>   - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
>   - Rationalized usage of CONFIG_HAS_* in device.h
>   - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
> ---
> Changes in V3:
>   - ifdef device tree related things.
>   - update the commit message
> ---
> Changes in V2:
> 	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
> 	- #ifdef PCI related things.
> 	- #ifdef ACPI related things.
> 	- rename DEVICE_GIC to DEVIC_IC.
> 	- rename #ifdef guards.
> 	- switch Arm and PPC to generic device.h
> 	- add SPDX tag
> 	- update the commit message
> 
> ---
>   xen/arch/arm/device.c                         |  15 ++-
>   xen/arch/arm/domain_build.c                   |   2 +-
>   xen/arch/arm/gic-v2.c                         |   4 +-
>   xen/arch/arm/gic-v3.c                         |   6 +-
>   xen/arch/arm/gic.c                            |   4 +-
>   xen/arch/arm/include/asm/Makefile             |   1 +
>   xen/arch/ppc/include/asm/Makefile             |   1 +
>   xen/arch/ppc/include/asm/device.h             |  53 --------
>   .../asm => include/asm-generic}/device.h      | 125 +++++++++++-------
>   9 files changed, 102 insertions(+), 109 deletions(-)
>   delete mode 100644 xen/arch/ppc/include/asm/device.h
>   rename xen/{arch/arm/include/asm => include/asm-generic}/device.h (79%)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 1f631d3274..affbe79f9a 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -16,7 +16,10 @@
>   #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)
> @@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
>       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;
> @@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class class, const void *data, int class
>   
>       return -EBADF;
>   }
> +#endif
>   
>   enum device_class device_get_class(const struct dt_device_node *dev)
>   {
> @@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>       struct map_range_data mr_data = {
>           .d = d,
>           .p2mt = p2mt,
> -        .skip_mapping = !own_device ||
> -                        (is_pci_passthrough_enabled() &&
> -                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
> +        .skip_mapping =
> +                        !own_device
> +#ifdef CONFIG_HAS_PCI
> +                        || (is_pci_passthrough_enabled() &&
> +                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))
> +#endif

So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not defined. 
It is not clear what's the actual problem of keeping 
DEVICE_PCI_HOSTBRIDGE available for every build.

In fact, we have tried to get away from #ifdef in order to make ensure 
we can catch any build failure without a randconfig (see use of 
IS_ENABLED() which would not apply work here).

[...]

> diff --git a/xen/arch/ppc/include/asm/device.h b/xen/arch/ppc/include/asm/device.h
> deleted file mode 100644
> index 8253e61d51..0000000000
> --- a/xen/arch/ppc/include/asm/device.h
> +++ /dev/null
> @@ -1,53 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -#ifndef __ASM_PPC_DEVICE_H__
> -#define __ASM_PPC_DEVICE_H__
> -
> -enum device_type
> -{
> -    DEV_DT,
> -    DEV_PCI,
> -};
> -
> -struct device {
> -    enum device_type type;
> -#ifdef CONFIG_HAS_DEVICE_TREE
> -    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> -#endif
> -};
> -
> -enum device_class
> -{
> -    DEVICE_SERIAL,
> -    DEVICE_IOMMU,
> -    DEVICE_PCI_HOSTBRIDGE,
> -    /* Use for error */
> -    DEVICE_UNKNOWN,
> -};
> -
> -struct device_desc {
> -    /* Device name */
> -    const char *name;
> -    /* Device class */
> -    enum device_class class;
> -    /* List of devices supported by this driver */
> -    const struct dt_device_match *dt_match;
> -    /*
> -     * Device initialization.
> -     *
> -     * -EAGAIN is used to indicate that device probing is deferred.
> -     */
> -    int (*init)(struct dt_device_node *dev, const void *data);
> -};
> -
> -typedef struct device device_t;
> -
> -#define DT_DEVICE_START(name_, namestr_, class_)                    \
> -static const struct device_desc __dev_desc_##name_ __used           \
> -__section(".dev.info") = {                                          \
> -    .name = namestr_,                                               \
> -    .class = class_,                                                \
> -
> -#define DT_DEVICE_END                                               \
> -};
> -
> -#endif /* __ASM_PPC_DEVICE_H__ */
> diff --git a/xen/arch/arm/include/asm/device.h b/xen/include/asm-generic/device.h
> similarity index 79%
> rename from xen/arch/arm/include/asm/device.h
> rename to xen/include/asm-generic/device.h
> index b5d451e087..063c448c4e 100644
> --- a/xen/arch/arm/include/asm/device.h
> +++ b/xen/include/asm-generic/device.h
> @@ -1,14 +1,37 @@
> -#ifndef __ASM_ARM_DEVICE_H
> -#define __ASM_ARM_DEVICE_H
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +#include <xen/stdbool.h>
>   
>   enum device_type
>   {
> +#ifdef CONFIG_HAS_DEVICE_TREE
>       DEV_DT,
> +#endif
> +
> +#ifdef CONFIG_HAS_PCI
>       DEV_PCI,
> +#endif
> +    DEV_TYPE_MAX,
Nobody is using it. AFAICT, this was introduced because enum may be 
empty if !HAS_DEVICE_TREE and !HAS_PCI. If so, can you add a comment 
explaining it on top?

The rest LGTM.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Oleksii]

Hi Julien,

On Thu, 2023-12-21 at 19:38 +0000, Julien Grall wrote:
> Hi,
> 
> On 20/12/2023 14:08, Oleksii Kurochko wrote:
> > Arm, PPC and RISC-V use the same device.h thereby device.h
> > was moved to asm-generic. Arm's device.h was taken as a base with
> > the following changes:
> >   - #ifdef PCI related things.
> >   - #ifdef ACPI related things.
> >   - Rename #ifdef guards.
> >   - Add SPDX tag.
> >   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
> >   - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
> > 
> > Also Arm and PPC are switched to asm-generic version of device.h
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > 
> >       Jan wrote the following:
> >         Overall I think there are too many changes done all in one
> > go here.
> >         But it's mostly Arm which is affected, so I'll leave
> > judging about that
> >         to the Arm maintainers.
> >       
> >       Arm maintainers, will it be fine for you to not split the
> > patch?
> 
> So in general I agree with Jan, patches should be kept small so they
> are 
> easy to review.
> 
> Given the discussion has been on-going for a while (we are at v6),  I
> will give an attempt to review the patch as-is. But in the future, 
> please try to split. The smaller it is, the easier to review. For
> code 
> movement and refactoring, I tend to first have a few refactoring
> patches 
> and then move the code in a separate patch. So it is easier to spot
> the 
> differences.
Thanks, I'll separate the patch.
> 
> > ---
> > Changes in V6:
> >   - Rebase only.
> > ---
> > Changes in V5:
> >    - Removed generated file: xen/include/headers++.chk.new
> >    - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for
> > PPC as
> >      CONFIG_HAS_DEVICE_TREE will be always used for PPC.
> > ---
> > Changes in V4:
> >   - Updated the commit message
> >   - Switched Arm and PPC to asm-generic version of device.h
> >   - Replaced HAS_PCI with CONFIG_HAS_PCI
> >   - ifdef-ing iommu filed of dev_archdata struct with
> > CONFIG_HAS_PASSTHROUGH
> >   - ifdef-ing iommu_fwspec of device struct with
> > CONFIG_HAS_PASSTHROUGH
> >   - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
> >   - Updated the commit message ( remove a note with question about
> >     if device.h should be in asm-generic or not )
> >   - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
> >   - Rationalized usage of CONFIG_HAS_* in device.h
> >   - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
> > ---
> > Changes in V3:
> >   - ifdef device tree related things.
> >   - update the commit message
> > ---
> > Changes in V2:
> > 	- take ( as common ) device.h from Arm as PPC and RISC-V
> > use it as a base.
> > 	- #ifdef PCI related things.
> > 	- #ifdef ACPI related things.
> > 	- rename DEVICE_GIC to DEVIC_IC.
> > 	- rename #ifdef guards.
> > 	- switch Arm and PPC to generic device.h
> > 	- add SPDX tag
> > 	- update the commit message
> > 
> > ---
> >   xen/arch/arm/device.c                         |  15 ++-
> >   xen/arch/arm/domain_build.c                   |   2 +-
> >   xen/arch/arm/gic-v2.c                         |   4 +-
> >   xen/arch/arm/gic-v3.c                         |   6 +-
> >   xen/arch/arm/gic.c                            |   4 +-
> >   xen/arch/arm/include/asm/Makefile             |   1 +
> >   xen/arch/ppc/include/asm/Makefile             |   1 +
> >   xen/arch/ppc/include/asm/device.h             |  53 --------
> >   .../asm => include/asm-generic}/device.h      | 125 +++++++++++--
> > -----
> >   9 files changed, 102 insertions(+), 109 deletions(-)
> >   delete mode 100644 xen/arch/ppc/include/asm/device.h
> >   rename xen/{arch/arm/include/asm => include/asm-generic}/device.h
> > (79%)
> > 
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index 1f631d3274..affbe79f9a 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -16,7 +16,10 @@
> >   #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)
> > @@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node
> > *dev, enum device_class class,
> >       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;
> > @@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class
> > class, const void *data, int class
> >   
> >       return -EBADF;
> >   }
> > +#endif
> >   
> >   enum device_class device_get_class(const struct dt_device_node
> > *dev)
> >   {
> > @@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct
> > dt_device_node *dev, p2m_type_t p2mt,
> >       struct map_range_data mr_data = {
> >           .d = d,
> >           .p2mt = p2mt,
> > -        .skip_mapping = !own_device ||
> > -                        (is_pci_passthrough_enabled() &&
> > -                        (device_get_class(dev) ==
> > DEVICE_PCI_HOSTBRIDGE)),
> > +        .skip_mapping =
> > +                        !own_device
> > +#ifdef CONFIG_HAS_PCI
> > +                        || (is_pci_passthrough_enabled() &&
> > +                        (device_get_class(dev) ==
> > DEVICE_PCI_HOSTBRIDGE))
> > +#endif
> 
> So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not
> defined. 
> It is not clear what's the actual problem of keeping 
> DEVICE_PCI_HOSTBRIDGE available for every build.
The only reason for that is that it seems to be used only in thecase when CONFIG_HAS_PCI is enabled ( probably not all archs will
have PCI support ). Generally,
I think it's okay not to use #ifdef DEVICE_PCI_HOSTBRIDGE to keep
the Arm code cleaner.

Does it make sense?
> 
> In fact, we have tried to get away from #ifdef in order to make
> ensure 
> we can catch any build failure without a randconfig (see use of 
> IS_ENABLED() which would not apply work here).
It would be nice to use IS_ENABLED but in this case still need to
something with definition of DEVICE_PCI_HOSTBRIDGE.

> 
> [...]
> 
> > diff --git a/xen/arch/ppc/include/asm/device.h
> > b/xen/arch/ppc/include/asm/device.h
> > deleted file mode 100644
> > index 8253e61d51..0000000000
> > --- a/xen/arch/ppc/include/asm/device.h
> > +++ /dev/null
> > @@ -1,53 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -#ifndef __ASM_PPC_DEVICE_H__
> > -#define __ASM_PPC_DEVICE_H__
> > -
> > -enum device_type
> > -{
> > -    DEV_DT,
> > -    DEV_PCI,
> > -};
> > -
> > -struct device {
> > -    enum device_type type;
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > -    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > -#endif
> > -};
> > -
> > -enum device_class
> > -{
> > -    DEVICE_SERIAL,
> > -    DEVICE_IOMMU,
> > -    DEVICE_PCI_HOSTBRIDGE,
> > -    /* Use for error */
> > -    DEVICE_UNKNOWN,
> > -};
> > -
> > -struct device_desc {
> > -    /* Device name */
> > -    const char *name;
> > -    /* Device class */
> > -    enum device_class class;
> > -    /* List of devices supported by this driver */
> > -    const struct dt_device_match *dt_match;
> > -    /*
> > -     * Device initialization.
> > -     *
> > -     * -EAGAIN is used to indicate that device probing is
> > deferred.
> > -     */
> > -    int (*init)(struct dt_device_node *dev, const void *data);
> > -};
> > -
> > -typedef struct device device_t;
> > -
> > -#define DT_DEVICE_START(name_, namestr_,
> > class_)                    \
> > -static const struct device_desc __dev_desc_##name_
> > __used           \
> > -__section(".dev.info") =
> > {                                          \
> > -    .name =
> > namestr_,                                               \
> > -    .class =
> > class_,                                                \
> > -
> > -#define
> > DT_DEVICE_END                                               \
> > -};
> > -
> > -#endif /* __ASM_PPC_DEVICE_H__ */
> > diff --git a/xen/arch/arm/include/asm/device.h b/xen/include/asm-
> > generic/device.h
> > similarity index 79%
> > rename from xen/arch/arm/include/asm/device.h
> > rename to xen/include/asm-generic/device.h
> > index b5d451e087..063c448c4e 100644
> > --- a/xen/arch/arm/include/asm/device.h
> > +++ b/xen/include/asm-generic/device.h
> > @@ -1,14 +1,37 @@
> > -#ifndef __ASM_ARM_DEVICE_H
> > -#define __ASM_ARM_DEVICE_H
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +#include <xen/stdbool.h>
> >   
> >   enum device_type
> >   {
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> >       DEV_DT,
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_PCI
> >       DEV_PCI,
> > +#endif
> > +    DEV_TYPE_MAX,
> Nobody is using it. AFAICT, this was introduced because enum may be 
> empty if !HAS_DEVICE_TREE and !HAS_PCI. If so, can you add a comment 
> explaining it on top?
Sure, thanks. I'll add the comment.

> 
> The rest LGTM.
> 
> Cheers,
> 
~ Oleksii

Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Julien Grall]

Hi Oleksii,

On 22/12/2023 13:16, Oleksii wrote:
> On Thu, 2023-12-21 at 19:38 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 20/12/2023 14:08, Oleksii Kurochko wrote:
>>> Arm, PPC and RISC-V use the same device.h thereby device.h
>>> was moved to asm-generic. Arm's device.h was taken as a base with
>>> the following changes:
>>>    - #ifdef PCI related things.
>>>    - #ifdef ACPI related things.
>>>    - Rename #ifdef guards.
>>>    - Add SPDX tag.
>>>    - #ifdef CONFIG_HAS_DEVICE_TREE related things.
>>>    - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
>>>
>>> Also Arm and PPC are switched to asm-generic version of device.h
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>>
>>>        Jan wrote the following:
>>>          Overall I think there are too many changes done all in one
>>> go here.
>>>          But it's mostly Arm which is affected, so I'll leave
>>> judging about that
>>>          to the Arm maintainers.
>>>        
>>>        Arm maintainers, will it be fine for you to not split the
>>> patch?
>>
>> So in general I agree with Jan, patches should be kept small so they
>> are
>> easy to review.
>>
>> Given the discussion has been on-going for a while (we are at v6),  I
>> will give an attempt to review the patch as-is. But in the future,
>> please try to split. The smaller it is, the easier to review. For
>> code
>> movement and refactoring, I tend to first have a few refactoring
>> patches
>> and then move the code in a separate patch. So it is easier to spot
>> the
>> differences.
> Thanks, I'll separate the patch.
>>
>>> ---
>>> Changes in V6:
>>>    - Rebase only.
>>> ---
>>> Changes in V5:
>>>     - Removed generated file: xen/include/headers++.chk.new
>>>     - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for
>>> PPC as
>>>       CONFIG_HAS_DEVICE_TREE will be always used for PPC.
>>> ---
>>> Changes in V4:
>>>    - Updated the commit message
>>>    - Switched Arm and PPC to asm-generic version of device.h
>>>    - Replaced HAS_PCI with CONFIG_HAS_PCI
>>>    - ifdef-ing iommu filed of dev_archdata struct with
>>> CONFIG_HAS_PASSTHROUGH
>>>    - ifdef-ing iommu_fwspec of device struct with
>>> CONFIG_HAS_PASSTHROUGH
>>>    - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
>>>    - Updated the commit message ( remove a note with question about
>>>      if device.h should be in asm-generic or not )
>>>    - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
>>>    - Rationalized usage of CONFIG_HAS_* in device.h
>>>    - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
>>> ---
>>> Changes in V3:
>>>    - ifdef device tree related things.
>>>    - update the commit message
>>> ---
>>> Changes in V2:
>>> 	- take ( as common ) device.h from Arm as PPC and RISC-V
>>> use it as a base.
>>> 	- #ifdef PCI related things.
>>> 	- #ifdef ACPI related things.
>>> 	- rename DEVICE_GIC to DEVIC_IC.
>>> 	- rename #ifdef guards.
>>> 	- switch Arm and PPC to generic device.h
>>> 	- add SPDX tag
>>> 	- update the commit message
>>>
>>> ---
>>>    xen/arch/arm/device.c                         |  15 ++-
>>>    xen/arch/arm/domain_build.c                   |   2 +-
>>>    xen/arch/arm/gic-v2.c                         |   4 +-
>>>    xen/arch/arm/gic-v3.c                         |   6 +-
>>>    xen/arch/arm/gic.c                            |   4 +-
>>>    xen/arch/arm/include/asm/Makefile             |   1 +
>>>    xen/arch/ppc/include/asm/Makefile             |   1 +
>>>    xen/arch/ppc/include/asm/device.h             |  53 --------
>>>    .../asm => include/asm-generic}/device.h      | 125 +++++++++++--
>>> -----
>>>    9 files changed, 102 insertions(+), 109 deletions(-)
>>>    delete mode 100644 xen/arch/ppc/include/asm/device.h
>>>    rename xen/{arch/arm/include/asm => include/asm-generic}/device.h
>>> (79%)
>>>
>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>> index 1f631d3274..affbe79f9a 100644
>>> --- a/xen/arch/arm/device.c
>>> +++ b/xen/arch/arm/device.c
>>> @@ -16,7 +16,10 @@
>>>    #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)
>>> @@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node
>>> *dev, enum device_class class,
>>>        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;
>>> @@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class
>>> class, const void *data, int class
>>>    
>>>        return -EBADF;
>>>    }
>>> +#endif
>>>    
>>>    enum device_class device_get_class(const struct dt_device_node
>>> *dev)
>>>    {
>>> @@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct
>>> dt_device_node *dev, p2m_type_t p2mt,
>>>        struct map_range_data mr_data = {
>>>            .d = d,
>>>            .p2mt = p2mt,
>>> -        .skip_mapping = !own_device ||
>>> -                        (is_pci_passthrough_enabled() &&
>>> -                        (device_get_class(dev) ==
>>> DEVICE_PCI_HOSTBRIDGE)),
>>> +        .skip_mapping =
>>> +                        !own_device
>>> +#ifdef CONFIG_HAS_PCI
>>> +                        || (is_pci_passthrough_enabled() &&
>>> +                        (device_get_class(dev) ==
>>> DEVICE_PCI_HOSTBRIDGE))
>>> +#endif
>>
>> So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not
>> defined.
>> It is not clear what's the actual problem of keeping
>> DEVICE_PCI_HOSTBRIDGE available for every build.
> The only reason for that is that it seems to be used only in thecase when CONFIG_HAS_PCI is enabled ( probably not all archs will
> have PCI support ). 

Possibly yes. But you will always need some compat layer unless you 
manage to get rid of any use of PCI (or any feature you don't want) 
within "common" code.

Here, it is not clear to me what you are effectively trying to protect 
against. IOW, what could go wrong if PCI_HOSTBRIDGE is available for 
everyone?

I know it means we would never return DEVICE_PCI_HOSTBRIDGE even if the 
device is actually a hostbridge. But the same is true if Xen doesn't 
have a driver for the hostbridge (not everyone are using ECAM).

So at the end of the day this is a trade-off between keep the code 
readable (from my POV) and reducing the amount of symbols/defines exposed.

This is also matching my view on Jan's proposal to protect the static 
keyword for first_valid_mfn with #ifdef. There is a limit in the amount 
of #ifdef I will tolerate.

I'd like to hear the opinion from the others.

> Generally,
> I think it's okay not to use #ifdef DEVICE_PCI_HOSTBRIDGE to keep
> the Arm code cleaner.
> 
> Does it make sense?

I don't quite understand your last sentence. Are you saying you would be 
ok to remove #ifdef CONFIG_HAS_PCI around DEVICE_PCI_HOSTBRIDGE?

Cheers,

-- 
Julien Grall

Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Oleksii]

> 
> > Generally,
> > I think it's okay not to use #ifdef DEVICE_PCI_HOSTBRIDGE to keep
> > the Arm code cleaner.
> > 
> > Does it make sense?
> 
> I don't quite understand your last sentence. Are you saying you would
> be 
> ok to remove #ifdef CONFIG_HAS_PCI around DEVICE_PCI_HOSTBRIDGE?
Yes, that is what I meant.


~ Oleksii