Re: [Qemu-devel] [PATCH RFC v4 06/20] target-arm: Move CPU feature flags out of CPUState

[Paul Brook]

> The internal CPU feature flags were only ever set in
> cpu_reset_model_id(). Therefore move their initialization into
> ARMCPUClass. We might want to tweak them in the future though (e.g.,
> -cpu cortex-r4,+fpu), so keep a copy in ARMCPU. This in turn means we
> need to infer features for both ARMCPUClass and ARMCPU, so move feature
> inference to arm_infer_features() and use macros to simplify it.
> 
> Since cpu.h defines ARMCPUState, which has been incorporated into
> ARMCPU, and tries to use arm_feature() in cpu_get_tb_cpu_state(),
> move arm_feature() to cpu-core.h and add a forward declaration.

I don't like how this has ended up.  You've got a confusing mix of 
decalarative and imperative code to set CPU features.

i.e. some cores set features in ARMCPU.class_init, others use ARMCPU.features.  
I'd much prefer doing everything in the former, and removing the latter.

Also, you must not assume that the feature bits will fit in a single word.
The features code is deliberately structured so that everything outside 
arm_feature and get_feature use the ARM_FEATURE_* enumeration.  This means 
underlying bitmap implementation can be easily changed/extended.  Your patch 
breaks that, making it inconvenient to expand beyond 32 features, and very 
hard to go beyond 64.  Even just considering currently available ARM variants, 
we're likely to hit these limits very soon.

Infering of additional features should probably be done in set_feature.

Paul