1. Patchew
  2. QEMU
  3. [PATCH v2 00/30] single-binary: start make hw/arm/ common
  4. View patch

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

Pierrick Bouvier posted 30 patches 10 months, 3 weeks ago
There is a newer version of this series
[PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Pierrick Bouvier 10 months, 3 weeks ago 
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;
-- 
2.39.5
Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Richard Henderson 10 months, 3 weeks ago 
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.


r~
Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Pierrick Bouvier 10 months, 3 weeks ago 
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~
Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Richard Henderson 10 months, 3 weeks ago 
On 3/24/25 14:11, Pierrick Bouvier wrote:
> 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.

Whoops, yes.

r~
Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Philippe Mathieu-Daudé 10 months, 2 weeks ago 
On 25/3/25 02:24, Richard Henderson wrote:
> On 3/24/25 14:11, Pierrick Bouvier wrote:
>> 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.
> 
> Whoops, yes.

If we go this way, can we consistently allow CONFIG_${HW_ACCEL}
(read "remove poisoned defs in config-poison.h)?
Re: [PATCH v2 20/30] target/arm/cpu: always define kvm related registers
Posted by Pierrick Bouvier 10 months, 2 weeks ago 
On 4/2/25 06:36, Philippe Mathieu-Daudé wrote:
> On 25/3/25 02:24, Richard Henderson wrote:
>> On 3/24/25 14:11, Pierrick Bouvier wrote:
>>> 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.
>>
>> Whoops, yes.
> 
> If we go this way, can we consistently allow CONFIG_${HW_ACCEL}
> (read "remove poisoned defs in config-poison.h)?

It would be safe to do this for CONFIG_TCG, which is applied to all 
compilation units (through config_host). And we'll do it when we meet a 
case that really needs it, not before. As long as the code can be 
cleaned up from those ifdefs, it's better.

However, it's not safe for all other CONFIG_${HW_ACCEL}, which are 
applied selectively on some targets (basically, for the pair {host == 
target}, when host supports this acceleration).
For them, the right fix is to make sure we call "{accel}_enabled()", 
expose the associated code, and eventually deal with missing symbols at 
link.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.