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