diff mbox series

[04/13] exec/cpu-all: allow to include specific cpu

Message ID 20250318045125.759259-5-pierrick.bouvier@linaro.org (mailing list archive)
State New
Headers show
Series single-binary: start make hw/arm/ common (boot.c) | expand

Commit Message

Pierrick Bouvier March 18, 2025, 4:51 a.m. UTC
Including "cpu.h" from code that is not compiled per target is ambiguous
by definition. Thus we introduce a conditional include, to allow every
architecture to set this, to point to the correct definition.

hw/X or target/X will now include directly "target/X/cpu.h", and
"target/X/cpu.h" will define CPU_INCLUDE to itself.
We already do this change for arm cpu as part of this commit.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/cpu-all.h | 4 ++++
 target/arm/cpu.h       | 2 ++
 2 files changed, 6 insertions(+)

Comments

Richard Henderson March 18, 2025, 10:11 p.m. UTC | #1
On 3/17/25 21:51, Pierrick Bouvier wrote:
> Including "cpu.h" from code that is not compiled per target is ambiguous
> by definition. Thus we introduce a conditional include, to allow every
> architecture to set this, to point to the correct definition.
> 
> hw/X or target/X will now include directly "target/X/cpu.h", and
> "target/X/cpu.h" will define CPU_INCLUDE to itself.
> We already do this change for arm cpu as part of this commit.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/exec/cpu-all.h | 4 ++++
>   target/arm/cpu.h       | 2 ++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 7c6c47c43ed..1a756c0cfb3 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -46,7 +46,11 @@
>   
>   CPUArchState *cpu_copy(CPUArchState *env);
>   
> +#ifdef CPU_INCLUDE
> +#include CPU_INCLUDE
> +#else
>   #include "cpu.h"
> +#endif
>   
>   #ifdef CONFIG_USER_ONLY
>   
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index a8177c6c2e8..7aeb012428c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -31,6 +31,8 @@
>   #include "target/arm/multiprocessing.h"
>   #include "target/arm/gtimer.h"
>   
> +#define CPU_INCLUDE "target/arm/cpu.h"
> +
>   #ifdef TARGET_AARCH64
>   #define KVM_HAVE_MCE_INJECTION 1
>   #endif

This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that 
you're trying to include by avoiding "cpu.h".


r~
Pierrick Bouvier March 18, 2025, 10:16 p.m. UTC | #2
On 3/18/25 15:11, Richard Henderson wrote:
> On 3/17/25 21:51, Pierrick Bouvier wrote:
>> Including "cpu.h" from code that is not compiled per target is ambiguous
>> by definition. Thus we introduce a conditional include, to allow every
>> architecture to set this, to point to the correct definition.
>>
>> hw/X or target/X will now include directly "target/X/cpu.h", and
>> "target/X/cpu.h" will define CPU_INCLUDE to itself.
>> We already do this change for arm cpu as part of this commit.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/exec/cpu-all.h | 4 ++++
>>    target/arm/cpu.h       | 2 ++
>>    2 files changed, 6 insertions(+)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index 7c6c47c43ed..1a756c0cfb3 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -46,7 +46,11 @@
>>    
>>    CPUArchState *cpu_copy(CPUArchState *env);
>>    
>> +#ifdef CPU_INCLUDE
>> +#include CPU_INCLUDE
>> +#else
>>    #include "cpu.h"
>> +#endif
>>    
>>    #ifdef CONFIG_USER_ONLY
>>    
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index a8177c6c2e8..7aeb012428c 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -31,6 +31,8 @@
>>    #include "target/arm/multiprocessing.h"
>>    #include "target/arm/gtimer.h"
>>    
>> +#define CPU_INCLUDE "target/arm/cpu.h"
>> +
>>    #ifdef TARGET_AARCH64
>>    #define KVM_HAVE_MCE_INJECTION 1
>>    #endif
> 
> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
> you're trying to include by avoiding "cpu.h".
> 

Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, 
relying on per target include path set by build system. Now we have 
common code, there is no "per target include path".

The other solutions are:
- build hw common libraries with per target include path, but I thought 
it was a good way to cleanup this, and not rely on this hidden 
dependency on the build system
- remove cpu.h inclusion from cpu-all.h, but it requires more 
modifications in other places.

I'm not sure which is the more desirable, compare to having this weird 
CPU_INCLUDE trick.

> 
> r~
Richard Henderson March 18, 2025, 10:21 p.m. UTC | #3
On 3/18/25 15:16, Pierrick Bouvier wrote:
>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>> you're trying to include by avoiding "cpu.h".
>>
> 
> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per 
> target include path set by build system.

So, another solution would be to fix the silly include loop?


r~
Pierrick Bouvier March 18, 2025, 10:25 p.m. UTC | #4
On 3/18/25 15:21, Richard Henderson wrote:
> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>> you're trying to include by avoiding "cpu.h".
>>>
>>
>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>> target include path set by build system.
> 
> So, another solution would be to fix the silly include loop?
>

If you're ok with it, I'm willing to remove cpu-all.h completely (moving 
tlb flags bits in a new header), and fixing missing includes everywhere.

I just wanted to make sure it's an acceptable path before spending too 
much time on it.

> 
> r~
Richard Henderson March 18, 2025, 10:36 p.m. UTC | #5
On 3/18/25 15:25, Pierrick Bouvier wrote:
> On 3/18/25 15:21, Richard Henderson wrote:
>> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>>> you're trying to include by avoiding "cpu.h".
>>>>
>>>
>>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>>> target include path set by build system.
>>
>> So, another solution would be to fix the silly include loop?
>>
> 
> If you're ok with it, I'm willing to remove cpu-all.h completely (moving tlb flags bits in 
> a new header), and fixing missing includes everywhere.
> 
> I just wanted to make sure it's an acceptable path before spending too much time on it.

I would very much like cpu-all.h to go away.

It looks like we have, on tcg-next:

(1) cpu_copy is linux-user only, and should go in linux-user/qemu.h.

(2) the TLB flags certainly deserve their own header.

(3) The QEMU_BUILD_BUG_ON assertions need not be done in a header,
     so long as there is *some* file that won't build if the assertions fail.
     Perhaps cpu-target.c is as good as any.


r~
Pierrick Bouvier March 18, 2025, 10:58 p.m. UTC | #6
On 3/18/25 15:36, Richard Henderson wrote:
> On 3/18/25 15:25, Pierrick Bouvier wrote:
>> On 3/18/25 15:21, Richard Henderson wrote:
>>> On 3/18/25 15:16, Pierrick Bouvier wrote:
>>>>> This doesn't make any sense to me.  CPU_INCLUDE is defined within the very file that
>>>>> you're trying to include by avoiding "cpu.h".
>>>>>
>>>>
>>>> Every target/X/cpu.h includes cpu-all.h, which includes "cpu.h" itself, relying on per
>>>> target include path set by build system.
>>>
>>> So, another solution would be to fix the silly include loop?
>>>
>>
>> If you're ok with it, I'm willing to remove cpu-all.h completely (moving tlb flags bits in
>> a new header), and fixing missing includes everywhere.
>>
>> I just wanted to make sure it's an acceptable path before spending too much time on it.
> 
> I would very much like cpu-all.h to go away.
> 

Deal, I will complete the work, while being based on your current series 
(v2).

> It looks like we have, on tcg-next:
> 
> (1) cpu_copy is linux-user only, and should go in linux-user/qemu.h.
> 
> (2) the TLB flags certainly deserve their own header.
> 
> (3) The QEMU_BUILD_BUG_ON assertions need not be done in a header,
>       so long as there is *some* file that won't build if the assertions fail.
>       Perhaps cpu-target.c is as good as any.
> 

Yes, I noticed it, and chose #ifdef COMPILING_PER_TARGET workaround to 
not make a choice of where to move it.

> 
> r~
diff mbox series

Patch

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 7c6c47c43ed..1a756c0cfb3 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -46,7 +46,11 @@ 
 
 CPUArchState *cpu_copy(CPUArchState *env);
 
+#ifdef CPU_INCLUDE
+#include CPU_INCLUDE
+#else
 #include "cpu.h"
+#endif
 
 #ifdef CONFIG_USER_ONLY
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a8177c6c2e8..7aeb012428c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -31,6 +31,8 @@ 
 #include "target/arm/multiprocessing.h"
 #include "target/arm/gtimer.h"
 
+#define CPU_INCLUDE "target/arm/cpu.h"
+
 #ifdef TARGET_AARCH64
 #define KVM_HAVE_MCE_INJECTION 1
 #endif