diff mbox series

[RFC,PATCH-for-10.1,11/39] hw/arm: Use full "target/arm/cpu.h" path to include target's "cpu.h"

Message ID 20250403235821.9909-12-philmd@linaro.org
State New
Headers show
Series single-binary: Make hw/arm/ common | expand

Commit Message

Philippe Mathieu-Daudé April 3, 2025, 11:57 p.m. UTC
We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.
Use the full path to "cpu.h": "target/arm/cpu.h".

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/arm/digic.h      | 2 +-
 include/hw/arm/fsl-imx6.h   | 2 +-
 include/hw/arm/fsl-imx6ul.h | 2 +-
 include/hw/arm/fsl-imx7.h   | 2 +-
 include/hw/arm/fsl-imx8mp.h | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

Comments

Pierrick Bouvier April 4, 2025, 6:20 p.m. UTC | #1
On 4/3/25 16:57, Philippe Mathieu-Daudé wrote:
> We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.

While this change is correct, this is not strictly needed.
With the current approach, using a set of common files per architecture, 
we can rely on this include to be present, and it does not block from 
having common files.

> Use the full path to "cpu.h": "target/arm/cpu.h".
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/arm/digic.h      | 2 +-
>   include/hw/arm/fsl-imx6.h   | 2 +-
>   include/hw/arm/fsl-imx6ul.h | 2 +-
>   include/hw/arm/fsl-imx7.h   | 2 +-
>   include/hw/arm/fsl-imx8mp.h | 2 +-
>   5 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h
> index 8f2735c284f..646802806e0 100644
> --- a/include/hw/arm/digic.h
> +++ b/include/hw/arm/digic.h
> @@ -18,7 +18,7 @@
>   #ifndef HW_ARM_DIGIC_H
>   #define HW_ARM_DIGIC_H
>   
> -#include "cpu.h"
> +#include "target/arm/cpu.h"
>   #include "hw/timer/digic-timer.h"
>   #include "hw/char/digic-uart.h"
>   #include "qom/object.h"
> diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
> index 124bbd478fd..0ac145cf6ba 100644
> --- a/include/hw/arm/fsl-imx6.h
> +++ b/include/hw/arm/fsl-imx6.h
> @@ -35,7 +35,7 @@
>   #include "hw/pci-host/designware.h"
>   #include "hw/or-irq.h"
>   #include "system/memory.h"
> -#include "cpu.h"
> +#include "target/arm/cpu.h"
>   #include "qom/object.h"
>   
>   #define TYPE_FSL_IMX6 "fsl-imx6"
> diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
> index 4e3209b25b2..f8f9c249a23 100644
> --- a/include/hw/arm/fsl-imx6ul.h
> +++ b/include/hw/arm/fsl-imx6ul.h
> @@ -34,7 +34,7 @@
>   #include "hw/usb/chipidea.h"
>   #include "hw/usb/imx-usb-phy.h"
>   #include "system/memory.h"
> -#include "cpu.h"
> +#include "target/arm/cpu.h"
>   #include "qom/object.h"
>   #include "qemu/units.h"
>   
> diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
> index aa7818c4999..6aedd2b80b5 100644
> --- a/include/hw/arm/fsl-imx7.h
> +++ b/include/hw/arm/fsl-imx7.h
> @@ -37,7 +37,7 @@
>   #include "hw/pci-host/designware.h"
>   #include "hw/usb/chipidea.h"
>   #include "hw/or-irq.h"
> -#include "cpu.h"
> +#include "target/arm/cpu.h"
>   #include "qom/object.h"
>   #include "qemu/units.h"
>   
> diff --git a/include/hw/arm/fsl-imx8mp.h b/include/hw/arm/fsl-imx8mp.h
> index bc97fc416eb..f20f9e53187 100644
> --- a/include/hw/arm/fsl-imx8mp.h
> +++ b/include/hw/arm/fsl-imx8mp.h
> @@ -9,7 +9,7 @@
>   #ifndef FSL_IMX8MP_H
>   #define FSL_IMX8MP_H
>   
> -#include "cpu.h"
> +#include "target/arm/cpu.h"
>   #include "hw/char/imx_serial.h"
>   #include "hw/gpio/imx_gpio.h"
>   #include "hw/i2c/imx_i2c.h"
Philippe Mathieu-Daudé April 4, 2025, 9:53 p.m. UTC | #2
On 4/4/25 20:20, Pierrick Bouvier wrote:
> On 4/3/25 16:57, Philippe Mathieu-Daudé wrote:
>> We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.
> 
> While this change is correct, this is not strictly needed.
> With the current approach, using a set of common files per architecture, 
> we can rely on this include to be present, and it does not block from 
> having common files.

Indeed, I rebased this commit from my heterogeneous branch.

I'll keeping carrying / rebasing it for various months, trying to
remember to not keeping posting it.

Or alternatively I'll post it in a separate "cleanup series", not
mentioning single-binary or heterogeneous emulation.

>> Use the full path to "cpu.h": "target/arm/cpu.h".
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/arm/digic.h      | 2 +-
>>   include/hw/arm/fsl-imx6.h   | 2 +-
>>   include/hw/arm/fsl-imx6ul.h | 2 +-
>>   include/hw/arm/fsl-imx7.h   | 2 +-
>>   include/hw/arm/fsl-imx8mp.h | 2 +-
>>   5 files changed, 5 insertions(+), 5 deletions(-)
Pierrick Bouvier April 5, 2025, 1:03 a.m. UTC | #3
On 4/4/25 14:53, Philippe Mathieu-Daudé wrote:
> On 4/4/25 20:20, Pierrick Bouvier wrote:
>> On 4/3/25 16:57, Philippe Mathieu-Daudé wrote:
>>> We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.
>>
>> While this change is correct, this is not strictly needed.
>> With the current approach, using a set of common files per architecture,
>> we can rely on this include to be present, and it does not block from
>> having common files.
> 
> Indeed, I rebased this commit from my heterogeneous branch.
> 
> I'll keeping carrying / rebasing it for various months, trying to
> remember to not keeping posting it.
> 
> Or alternatively I'll post it in a separate "cleanup series", not
> mentioning single-binary or heterogeneous emulation.
> 

My point was not "please post that later", but simply to say those 
changes are not needed, now or in the future.
We can have a specific include path for various files without 
compromising the single binary/heterogenenous, as long as we compile it 
only once.

So we don't need to remove cpu.h inclusion in target related code.

>>> Use the full path to "cpu.h": "target/arm/cpu.h".
>>>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    include/hw/arm/digic.h      | 2 +-
>>>    include/hw/arm/fsl-imx6.h   | 2 +-
>>>    include/hw/arm/fsl-imx6ul.h | 2 +-
>>>    include/hw/arm/fsl-imx7.h   | 2 +-
>>>    include/hw/arm/fsl-imx8mp.h | 2 +-
>>>    5 files changed, 5 insertions(+), 5 deletions(-)
>
Philippe Mathieu-Daudé April 5, 2025, 2:27 p.m. UTC | #4
On 5/4/25 03:03, Pierrick Bouvier wrote:
> On 4/4/25 14:53, Philippe Mathieu-Daudé wrote:
>> On 4/4/25 20:20, Pierrick Bouvier wrote:
>>> On 4/3/25 16:57, Philippe Mathieu-Daudé wrote:
>>>> We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.
>>>
>>> While this change is correct, this is not strictly needed.
>>> With the current approach, using a set of common files per architecture,
>>> we can rely on this include to be present, and it does not block from
>>> having common files.
>>
>> Indeed, I rebased this commit from my heterogeneous branch.
>>
>> I'll keeping carrying / rebasing it for various months, trying to
>> remember to not keeping posting it.
>>
>> Or alternatively I'll post it in a separate "cleanup series", not
>> mentioning single-binary or heterogeneous emulation.
>>
> 
> My point was not "please post that later", but simply to say those 
> changes are not needed, now or in the future.
> We can have a specific include path for various files without 
> compromising the single binary/heterogenenous, as long as we compile it 
> only once.

Incorrect, this is required for heterogenenous emulation.

> So we don't need to remove cpu.h inclusion in target related code.
> 
>>>> Use the full path to "cpu.h": "target/arm/cpu.h".
>>>>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    include/hw/arm/digic.h      | 2 +-
>>>>    include/hw/arm/fsl-imx6.h   | 2 +-
>>>>    include/hw/arm/fsl-imx6ul.h | 2 +-
>>>>    include/hw/arm/fsl-imx7.h   | 2 +-
>>>>    include/hw/arm/fsl-imx8mp.h | 2 +-
>>>>    5 files changed, 5 insertions(+), 5 deletions(-)
>>
>
Pierrick Bouvier April 7, 2025, 5:13 p.m. UTC | #5
On 4/5/25 07:27, Philippe Mathieu-Daudé wrote:
> On 5/4/25 03:03, Pierrick Bouvier wrote:
>> On 4/4/25 14:53, Philippe Mathieu-Daudé wrote:
>>> On 4/4/25 20:20, Pierrick Bouvier wrote:
>>>> On 4/3/25 16:57, Philippe Mathieu-Daudé wrote:
>>>>> We would like to get rid of '-I target/$ARCH/' in the CPPFLAGS.
>>>>
>>>> While this change is correct, this is not strictly needed.
>>>> With the current approach, using a set of common files per architecture,
>>>> we can rely on this include to be present, and it does not block from
>>>> having common files.
>>>
>>> Indeed, I rebased this commit from my heterogeneous branch.
>>>
>>> I'll keeping carrying / rebasing it for various months, trying to
>>> remember to not keeping posting it.
>>>
>>> Or alternatively I'll post it in a separate "cleanup series", not
>>> mentioning single-binary or heterogeneous emulation.
>>>
>>
>> My point was not "please post that later", but simply to say those
>> changes are not needed, now or in the future.
>> We can have a specific include path for various files without
>> compromising the single binary/heterogenenous, as long as we compile it
>> only once.
> 
> Incorrect, this is required for heterogenenous emulation.
> 

There are probably two different topics here.

If by "this", you mean the current change (include explicit 
target/X/cpu.h), then no, it's not required.
Changing include "cpu.h" to include "target/arm/cpu.h" has absolutely no 
effect for the common files in hw/arm, as they are already compiled with 
-I target/arm. It produces the exact same compilation unit.
I did the same change as you do here when working on hw/arm, and finally 
understood it had no effect, so I reverted it, and simply let the 
original include "cpu.h".

If you were talking about accessing arm/cpu.h features, then I still 
don't see why it's needed. There is no reason any code out of hw/arm or 
target/arm should access this. If I missed something, feel free to post 
a concrete example of where it's needed.

In any case, if any hw code relies on cpu.h inclusion, it will not 
compile when we make it common, because cpu.h won't be found.
At this time, and only at this time, then we'll extract the expected 
interface, and expose it to common code. But before we meet the 
situation, I don't think we should do any of those changes.

Pierrick
diff mbox series

Patch

diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h
index 8f2735c284f..646802806e0 100644
--- a/include/hw/arm/digic.h
+++ b/include/hw/arm/digic.h
@@ -18,7 +18,7 @@ 
 #ifndef HW_ARM_DIGIC_H
 #define HW_ARM_DIGIC_H
 
-#include "cpu.h"
+#include "target/arm/cpu.h"
 #include "hw/timer/digic-timer.h"
 #include "hw/char/digic-uart.h"
 #include "qom/object.h"
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 124bbd478fd..0ac145cf6ba 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -35,7 +35,7 @@ 
 #include "hw/pci-host/designware.h"
 #include "hw/or-irq.h"
 #include "system/memory.h"
-#include "cpu.h"
+#include "target/arm/cpu.h"
 #include "qom/object.h"
 
 #define TYPE_FSL_IMX6 "fsl-imx6"
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 4e3209b25b2..f8f9c249a23 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -34,7 +34,7 @@ 
 #include "hw/usb/chipidea.h"
 #include "hw/usb/imx-usb-phy.h"
 #include "system/memory.h"
-#include "cpu.h"
+#include "target/arm/cpu.h"
 #include "qom/object.h"
 #include "qemu/units.h"
 
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index aa7818c4999..6aedd2b80b5 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -37,7 +37,7 @@ 
 #include "hw/pci-host/designware.h"
 #include "hw/usb/chipidea.h"
 #include "hw/or-irq.h"
-#include "cpu.h"
+#include "target/arm/cpu.h"
 #include "qom/object.h"
 #include "qemu/units.h"
 
diff --git a/include/hw/arm/fsl-imx8mp.h b/include/hw/arm/fsl-imx8mp.h
index bc97fc416eb..f20f9e53187 100644
--- a/include/hw/arm/fsl-imx8mp.h
+++ b/include/hw/arm/fsl-imx8mp.h
@@ -9,7 +9,7 @@ 
 #ifndef FSL_IMX8MP_H
 #define FSL_IMX8MP_H
 
-#include "cpu.h"
+#include "target/arm/cpu.h"
 #include "hw/char/imx_serial.h"
 #include "hw/gpio/imx_gpio.h"
 #include "hw/i2c/imx_i2c.h"