include/hw/misc/xlnx-zynqmp-apu-ctrl.h | 2 +- target/arm/arm-powerctl.h | 2 -- hw/misc/imx6_src.c | 2 +- hw/misc/iotkit-sysctl.c | 1 - hw/misc/meson.build | 8 ++++---- 5 files changed, 6 insertions(+), 9 deletions(-)
By removing #include "kvm-consts.h" from arm-powerctl.h (seems not to
be required there) and adjusting the header includes in some files, we
can move them from specific_ss into softmmu_ss, so that they only need
to be compiled once and not for qemu-system-arm and qemu-system-aarch64
individually.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
include/hw/misc/xlnx-zynqmp-apu-ctrl.h | 2 +-
target/arm/arm-powerctl.h | 2 --
hw/misc/imx6_src.c | 2 +-
hw/misc/iotkit-sysctl.c | 1 -
hw/misc/meson.build | 8 ++++----
5 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/include/hw/misc/xlnx-zynqmp-apu-ctrl.h b/include/hw/misc/xlnx-zynqmp-apu-ctrl.h
index b8ca9434af..c3bf3c1583 100644
--- a/include/hw/misc/xlnx-zynqmp-apu-ctrl.h
+++ b/include/hw/misc/xlnx-zynqmp-apu-ctrl.h
@@ -13,7 +13,7 @@
#include "hw/sysbus.h"
#include "hw/register.h"
-#include "target/arm/cpu.h"
+#include "target/arm/cpu-qom.h"
#define TYPE_XLNX_ZYNQMP_APU_CTRL "xlnx.apu-ctrl"
OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPAPUCtrl, XLNX_ZYNQMP_APU_CTRL)
diff --git a/target/arm/arm-powerctl.h b/target/arm/arm-powerctl.h
index 37c8a04f0a..35e048ce14 100644
--- a/target/arm/arm-powerctl.h
+++ b/target/arm/arm-powerctl.h
@@ -11,8 +11,6 @@
#ifndef QEMU_ARM_POWERCTL_H
#define QEMU_ARM_POWERCTL_H
-#include "kvm-consts.h"
-
#define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS
#define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS
#define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON
diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
index 7b0e968804..a9c64d06eb 100644
--- a/hw/misc/imx6_src.c
+++ b/hw/misc/imx6_src.c
@@ -15,7 +15,7 @@
#include "qemu/log.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
-#include "arm-powerctl.h"
+#include "target/arm/arm-powerctl.h"
#include "hw/core/cpu.h"
#ifndef DEBUG_IMX6_SRC
diff --git a/hw/misc/iotkit-sysctl.c b/hw/misc/iotkit-sysctl.c
index 7147e2f84e..e664215ee6 100644
--- a/hw/misc/iotkit-sysctl.c
+++ b/hw/misc/iotkit-sysctl.c
@@ -30,7 +30,6 @@
#include "hw/qdev-properties.h"
#include "hw/arm/armsse-version.h"
#include "target/arm/arm-powerctl.h"
-#include "target/arm/cpu.h"
REG32(SECDBGSTAT, 0x0)
REG32(SECDBGSET, 0x4)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 95268eddc0..9ca6bf1d17 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -84,8 +84,8 @@ softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files(
))
softmmu_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c'))
softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c'))
-specific_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-crf.c'))
-specific_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-apu-ctrl.c'))
+softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-crf.c'))
+softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-apu-ctrl.c'))
specific_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-crl.c'))
softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files(
'xlnx-versal-xramc.c',
@@ -126,8 +126,8 @@ softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
-specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
-specific_ss.add(when: 'CONFIG_IOTKIT_SYSCTL', if_true: files('iotkit-sysctl.c'))
+softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
+softmmu_ss.add(when: 'CONFIG_IOTKIT_SYSCTL', if_true: files('iotkit-sysctl.c'))
specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c'))
--
2.31.1
On Wed, 30 Nov 2022 at 11:16, Thomas Huth <thuth@redhat.com> wrote: > > By removing #include "kvm-consts.h" from arm-powerctl.h (seems not to > be required there) and adjusting the header includes in some files, we > can move them from specific_ss into softmmu_ss, so that they only need > to be compiled once and not for qemu-system-arm and qemu-system-aarch64 > individually. > --- a/target/arm/arm-powerctl.h > +++ b/target/arm/arm-powerctl.h > @@ -11,8 +11,6 @@ > #ifndef QEMU_ARM_POWERCTL_H > #define QEMU_ARM_POWERCTL_H > > -#include "kvm-consts.h" > - > #define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS > #define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS > #define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON kvm-consts.h is where QEMU_PSCI_RET_SUCCESS etc are defined. So while the #include isn't strictly needed for compilation to work because arm-powerctl.h only creates the #define and doesn't use it, it does mean that any source file that uses the QEMU_ARM_POWERCTL_* now needs to include kvm-consts.h somehow itself. (Usually this is going to happen implicitly via target/arm/cpu.h, I think.) I guess this is worth living with for the benefit of not compiling things twice. It could probably be untangled a little by e.g. moving the PSCI constants into their own header rather than defining them in kvm-consts.h, but I'm not sure if it's worth the effort right now. > diff --git a/hw/misc/meson.build b/hw/misc/meson.build > index 95268eddc0..9ca6bf1d17 100644 > --- a/hw/misc/meson.build > +++ b/hw/misc/meson.build > @@ -84,8 +84,8 @@ softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files( > )) > softmmu_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c')) > softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c')) > -specific_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-crf.c')) > -specific_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-apu-ctrl.c')) > +softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-crf.c')) > +softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx-zynqmp-apu-ctrl.c')) > specific_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-crl.c')) > softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files( > 'xlnx-versal-xramc.c', > @@ -126,8 +126,8 @@ softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c')) > > specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c')) > > -specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c')) > -specific_ss.add(when: 'CONFIG_IOTKIT_SYSCTL', if_true: files('iotkit-sysctl.c')) > +softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c')) This file could now be listed in the "softmmu_ss.add(when: 'CONFIG_IMX', if_true: files(...)" list earlier in the file. > +softmmu_ss.add(when: 'CONFIG_IOTKIT_SYSCTL', if_true: files('iotkit-sysctl.c')) This line could be moved up to next to the other CONFIG_IOTKIT_* lines. > specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c')) thanks -- PMM
On 01/12/2022 12.55, Peter Maydell wrote: > On Wed, 30 Nov 2022 at 11:16, Thomas Huth <thuth@redhat.com> wrote: >> >> By removing #include "kvm-consts.h" from arm-powerctl.h (seems not to >> be required there) and adjusting the header includes in some files, we >> can move them from specific_ss into softmmu_ss, so that they only need >> to be compiled once and not for qemu-system-arm and qemu-system-aarch64 >> individually. > >> --- a/target/arm/arm-powerctl.h >> +++ b/target/arm/arm-powerctl.h >> @@ -11,8 +11,6 @@ >> #ifndef QEMU_ARM_POWERCTL_H >> #define QEMU_ARM_POWERCTL_H >> >> -#include "kvm-consts.h" >> - >> #define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS >> #define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS >> #define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON > > kvm-consts.h is where QEMU_PSCI_RET_SUCCESS etc are defined. > So while the #include isn't strictly needed for compilation to work > because arm-powerctl.h only creates the #define and doesn't use it, > it does mean that any source file that uses the QEMU_ARM_POWERCTL_* > now needs to include kvm-consts.h somehow itself. (Usually this is > going to happen implicitly via target/arm/cpu.h, I think.) > > I guess this is worth living with for the benefit of not > compiling things twice. It could probably be untangled a little > by e.g. moving the PSCI constants into their own header rather > than defining them in kvm-consts.h, but I'm not sure if it's > worth the effort right now. Hmm, do we really need these QEMU_ARM_POWERCTL* redefinitions? They seem hardly to be used outside of the arm-powerctl.[ch] files: $ grep -r QEMU_ARM_POWERCTL * | grep -v target/arm/arm-powerctl hw/misc/allwinner-cpucfg.c: if (ret != QEMU_ARM_POWERCTL_RET_SUCCESS) { target/arm/hvf/hvf.c: assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS); target/arm/psci.c: assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS); ... so maybe we could rather change those spots to use the QEMU_PSCI_* constants instead? ... or since they basically only check for success, we could maybe use "if (ret) ..." and "assert(!ret)" there? Thomas
On Fri, 2 Dec 2022 at 12:25, Thomas Huth <thuth@redhat.com> wrote: > > On 01/12/2022 12.55, Peter Maydell wrote: > > On Wed, 30 Nov 2022 at 11:16, Thomas Huth <thuth@redhat.com> wrote: > >> #define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS > >> #define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS > >> #define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON > > > > kvm-consts.h is where QEMU_PSCI_RET_SUCCESS etc are defined. > > So while the #include isn't strictly needed for compilation to work > > because arm-powerctl.h only creates the #define and doesn't use it, > > it does mean that any source file that uses the QEMU_ARM_POWERCTL_* > > now needs to include kvm-consts.h somehow itself. (Usually this is > > going to happen implicitly via target/arm/cpu.h, I think.) > > > > I guess this is worth living with for the benefit of not > > compiling things twice. It could probably be untangled a little > > by e.g. moving the PSCI constants into their own header rather > > than defining them in kvm-consts.h, but I'm not sure if it's > > worth the effort right now. > > Hmm, do we really need these QEMU_ARM_POWERCTL* redefinitions? > They seem hardly to be used outside of the arm-powerctl.[ch] > files: > > $ grep -r QEMU_ARM_POWERCTL * | grep -v target/arm/arm-powerctl > hw/misc/allwinner-cpucfg.c: if (ret != QEMU_ARM_POWERCTL_RET_SUCCESS) { > target/arm/hvf/hvf.c: assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS); > target/arm/psci.c: assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS); > > ... so maybe we could rather change those spots to use the QEMU_PSCI_* > constants instead? ... or since they basically only check for success, > we could maybe use "if (ret) ..." and "assert(!ret)" there? I see you've found a neat way to avoid this problem, but for the record, the reason the two sets of constant names are different is because these are two separate levels of API. The PSCI values are required to be those values by the PSCI specification. The ARM_POWERCTL values are just part of a within-QEMU API that is used both by our PSCI implementation and also by some non-PSCI power-control devices, so conceptually it shouldn't use PSCI constants at all. However we assume in our PSCI implementation that we can just pass through an Arm powerctl return code as a PSCI return code to the guest, and so we want at compile time to know that in fact we picked the same numbers for each. In theory you could separate them the two sets of constant definitions and then compile-time-assert in the PSCI implementation code that they have the same values, or you could really separate them out and then have the PSCI implementation code (that's the two cases in target/arm) do a runtime conversion between an ARM_POWERCTL return value and the appropriate PSCI return value. The current setup exists partly because we started with only a PSCI implementation, and then abstracted out the "start/stop a CPU" functionality into its own within-QEMU API so other power-control devices could use it. -- PMM
On 02/12/2022 13.25, Thomas Huth wrote: > On 01/12/2022 12.55, Peter Maydell wrote: >> On Wed, 30 Nov 2022 at 11:16, Thomas Huth <thuth@redhat.com> wrote: >>> >>> By removing #include "kvm-consts.h" from arm-powerctl.h (seems not to >>> be required there) and adjusting the header includes in some files, we >>> can move them from specific_ss into softmmu_ss, so that they only need >>> to be compiled once and not for qemu-system-arm and qemu-system-aarch64 >>> individually. >> >>> --- a/target/arm/arm-powerctl.h >>> +++ b/target/arm/arm-powerctl.h >>> @@ -11,8 +11,6 @@ >>> #ifndef QEMU_ARM_POWERCTL_H >>> #define QEMU_ARM_POWERCTL_H >>> >>> -#include "kvm-consts.h" >>> - >>> #define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS >>> #define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS >>> #define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON >> >> kvm-consts.h is where QEMU_PSCI_RET_SUCCESS etc are defined. >> So while the #include isn't strictly needed for compilation to work >> because arm-powerctl.h only creates the #define and doesn't use it, >> it does mean that any source file that uses the QEMU_ARM_POWERCTL_* >> now needs to include kvm-consts.h somehow itself. (Usually this is >> going to happen implicitly via target/arm/cpu.h, I think.) Thinking about this a little bit more (and finally having a look at the contents of kvm-consts.h itself) ... I think there might be an even more elegant solution here: We could maybe apply the "#ifdef NEED_CPU_H" trick in kvm-consts.h (like it is e.g. done in include/sysemu/kvm.h) to avoid to touch the poisoned CONFIG_KVM macro in common code ... I'll give it a try, and if it works, I'll use that method for my v2 of this patch. Thomas
© 2016 - 2024 Red Hat, Inc.