Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/23/25 12:37, Richard Henderson wrote:
> On 3/20/25 15:29, Pierrick Bouvier wrote:
>> This does not hurt, even if they are not used.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/arm/cpu.h | 2 --
>>    1 file changed, 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index a8a1a8faf6b..ab7412772bc 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -971,7 +971,6 @@ struct ArchCPU {
>>         */
>>        uint32_t kvm_target;
>>    
>> -#ifdef CONFIG_KVM
>>        /* KVM init features for this CPU */
>>        uint32_t kvm_init_features[7];
>>    
>> @@ -984,7 +983,6 @@ struct ArchCPU {
>>    
>>        /* KVM steal time */
>>        OnOffAuto kvm_steal_time;
>> -#endif /* CONFIG_KVM */
>>    
>>        /* Uniprocessor system with MP extensions */
>>        bool mp_is_up;
> 
> I'm not sure what this achieves?   CONFIG_KVM is a configure-time selection.
>

CONFIG_KVM is a poisoned identifier.
It's included via config-target.h, and not config-host.h. So common code 
relying on it might do the wrong thing.
As well, its presence is conditioned by target architecture (see 
meson.build), so it can't be enabled for all targets.

For this patch, it's only cpu definition, but for code based on 
CONFIG_KVM/TCG/HVF/XEN, we should probably check {accel}_enabled() 
accordingly.

However, at the moment, I'm not sure what is the best way to deal with 
it for common code, as {accel}_enabled() symbol can only be present once 
in the end.

> 
> r~