Message ID | 20250403235821.9909-12-philmd@linaro.org |
---|---|
State | New |
Headers | show |
Series | single-binary: Make hw/arm/ common | expand |
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"
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(-)
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(-) >
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(-) >> >
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 --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"