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

[Thomas Huth <thuth@redhat.com>]

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