Re: [Qemu-devel] [RFC PATCH 1/3] target/arm: Make cpu_register() and set_feature() available for other files

[Peter Maydell]

On Tue, 3 Sep 2019 at 16:54, Thomas Huth <address@hidden> wrote:
>
> Move the common set_feature() and unset_feature() functions from cpu.c and
> cpu64.c to cpu.h, and make cpu_register() (renamed to arm_cpu_register())
> available from there, too, so we can register CPUs also from other files
> in the future.
>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  target/arm/cpu.c   | 20 ++------------------
>  target/arm/cpu.h   | 18 ++++++++++++++++++
>  target/arm/cpu64.c | 16 ----------------
>  3 files changed, 20 insertions(+), 34 deletions(-)
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 0981303170..c5007edf1f 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3600,4 +3600,22 @@ static inline bool isar_feature_aa64_bti(const 
> ARMISARegisters *id)
>  #define cpu_isar_feature(name, cpu) \
>      ({ ARMCPU *cpu_ = (cpu); isar_feature_##name(&cpu_->isar); })
>
> +static inline void set_feature(CPUARMState *env, int feature)
> +{
> +    env->features |= 1ULL << feature;
> +}
> +
> +static inline void unset_feature(CPUARMState *env, int feature)
> +{
> +    env->features &= ~(1ULL << feature);
> +}

I think these function names are too generic to have in a header
like cpu.h which is used all across the codebase. (For instance
target/arm/kvm64.c now has both a local set_feature() function
and this one from the header.)

Can they go in target/arm/internals.h instead?
The set of code that should be caring about setting
feature bits should be pretty small.

Maybe also they should be renamed.

thanks
-- PMM