| 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 |
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~
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~
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~
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~
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~
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 --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
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(+)