Re: [PATCH for-8.0] hw/misc: Move some arm-related files from specific_ss into softmmu_ss

[Peter Maydell]

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