| Message ID | 20170912103622.18562-8-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Clean-up traps.c | expand |
On Tue, 12 Sep 2017, Julien Grall wrote: > Currently, cpregs.h is included in pretty much every files even for > arm64. However, the only use for arm64 is when emulating co-processors. > > For arm32, all users of processor.h expect cpregs.h to be included in > order to access co-processors. So move the inclusion in > asm-arm/arm32/processor.h. > > cpregs.h will also be directly included in the co-processors emulation > to accommodate arm64. > > Signed-off-by: Julien Grall <julien.grall@arm.com> I can see that the patch works and does what you describe, but what is the benefit? OK, we remove #include <asm/cpregs.h> from asm-arm/processor.h, but then we have to add it to vcpreg.c, vgic-v3.c, vtimer.c, and arm32/processor.h. Is there a long term benefit? What prompted you into writing this patch? > --- > Changes in v2: > - Update commit message > --- > xen/arch/arm/smp.c | 1 - > xen/arch/arm/vcpreg.c | 1 + > xen/arch/arm/vgic-v3.c | 1 + > xen/arch/arm/vtimer.c | 2 ++ > xen/include/asm-arm/arm32/processor.h | 2 ++ > xen/include/asm-arm/percpu.h | 1 - > xen/include/asm-arm/processor.h | 1 - > 7 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c > index e7df0874d6..554f4992e6 100644 > --- a/xen/arch/arm/smp.c > +++ b/xen/arch/arm/smp.c > @@ -1,6 +1,5 @@ > #include <asm/system.h> > #include <asm/smp.h> > -#include <asm/cpregs.h> > #include <asm/page.h> > #include <asm/gic.h> > #include <asm/flushtlb.h> > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > index f3b08403fb..e363183ba8 100644 > --- a/xen/arch/arm/vcpreg.c > +++ b/xen/arch/arm/vcpreg.c > @@ -18,6 +18,7 @@ > > #include <xen/sched.h> > > +#include <asm/cpregs.h> > #include <asm/current.h> > #include <asm/regs.h> > #include <asm/traps.h> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index cbeac28b28..a0cf993d13 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -26,6 +26,7 @@ > #include <xen/softirq.h> > #include <xen/sizes.h> > > +#include <asm/cpregs.h> > #include <asm/current.h> > #include <asm/gic_v3_defs.h> > #include <asm/gic_v3_its.h> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > index 9c7e8f441c..0460962f08 100644 > --- a/xen/arch/arm/vtimer.c > +++ b/xen/arch/arm/vtimer.c > @@ -22,6 +22,7 @@ > #include <xen/sched.h> > #include <xen/timer.h> > > +#include <asm/cpregs.h> > #include <asm/div64.h> > #include <asm/gic.h> > #include <asm/irq.h> > @@ -29,6 +30,7 @@ > #include <asm/time.h> > #include <asm/vgic.h> > #include <asm/vreg.h> > +#include <asm/regs.h> > > /* > * Check if regs is allowed access, user_gate is tail end of a > diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h > index 68cc82147e..fb330812af 100644 > --- a/xen/include/asm-arm/arm32/processor.h > +++ b/xen/include/asm-arm/arm32/processor.h > @@ -1,6 +1,8 @@ > #ifndef __ASM_ARM_ARM32_PROCESSOR_H > #define __ASM_ARM_ARM32_PROCESSOR_H > > +#include <asm/cpregs.h> > + > #define ACTLR_CAXX_SMP (1<<6) > > #ifndef __ASSEMBLY__ > diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h > index 7968532462..cdf64e0f77 100644 > --- a/xen/include/asm-arm/percpu.h > +++ b/xen/include/asm-arm/percpu.h > @@ -4,7 +4,6 @@ > #ifndef __ASSEMBLY__ > > #include <xen/types.h> > -#include <asm/cpregs.h> > #if defined(CONFIG_ARM_32) > # include <asm/arm32/processor.h> > #elif defined(CONFIG_ARM_64) > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index d791c12c9c..cd45e5f48f 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -1,7 +1,6 @@ > #ifndef __ASM_ARM_PROCESSOR_H > #define __ASM_ARM_PROCESSOR_H > > -#include <asm/cpregs.h> > #ifndef __ASSEMBLY__ > #include <xen/types.h> > #endif > -- > 2.11.0 >
Hi Stefano, On 09/12/2017 10:53 PM, Stefano Stabellini wrote: > On Tue, 12 Sep 2017, Julien Grall wrote: >> Currently, cpregs.h is included in pretty much every files even for >> arm64. However, the only use for arm64 is when emulating co-processors. >> >> For arm32, all users of processor.h expect cpregs.h to be included in >> order to access co-processors. So move the inclusion in >> asm-arm/arm32/processor.h. >> >> cpregs.h will also be directly included in the co-processors emulation >> to accommodate arm64. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > I can see that the patch works and does what you describe, but what is > the benefit? OK, we remove #include <asm/cpregs.h> from > asm-arm/processor.h, but then we have to add it to vcpreg.c, vgic-v3.c, > vtimer.c, and arm32/processor.h. Is there a long term benefit? What > prompted you into writing this patch? asm/processor.h is included indirectly by every single file of the source code. It seems that we use it as a placeholder for anything rather than properly splitting in different headers. For instance it contains all the definitions of the registers, the traps vector, SMC call, traps prototype... For most of those stuff only a limited of files really care about it. So we are polluting the name space of each file for no real benefits. On AArch64, cpregs.h is only useful when emulating co-processor gregisters. So there are no point to include it in a main header that will be pulled by 100s files just for the benetifs of 4 files. This patch is only a first attempt of clean-up processor.h. Ideally we should have more patch to split it. Cheers, > > >> --- >> Changes in v2: >> - Update commit message >> --- >> xen/arch/arm/smp.c | 1 - >> xen/arch/arm/vcpreg.c | 1 + >> xen/arch/arm/vgic-v3.c | 1 + >> xen/arch/arm/vtimer.c | 2 ++ >> xen/include/asm-arm/arm32/processor.h | 2 ++ >> xen/include/asm-arm/percpu.h | 1 - >> xen/include/asm-arm/processor.h | 1 - >> 7 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c >> index e7df0874d6..554f4992e6 100644 >> --- a/xen/arch/arm/smp.c >> +++ b/xen/arch/arm/smp.c >> @@ -1,6 +1,5 @@ >> #include <asm/system.h> >> #include <asm/smp.h> >> -#include <asm/cpregs.h> >> #include <asm/page.h> >> #include <asm/gic.h> >> #include <asm/flushtlb.h> >> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >> index f3b08403fb..e363183ba8 100644 >> --- a/xen/arch/arm/vcpreg.c >> +++ b/xen/arch/arm/vcpreg.c >> @@ -18,6 +18,7 @@ >> >> #include <xen/sched.h> >> >> +#include <asm/cpregs.h> >> #include <asm/current.h> >> #include <asm/regs.h> >> #include <asm/traps.h> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index cbeac28b28..a0cf993d13 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -26,6 +26,7 @@ >> #include <xen/softirq.h> >> #include <xen/sizes.h> >> >> +#include <asm/cpregs.h> >> #include <asm/current.h> >> #include <asm/gic_v3_defs.h> >> #include <asm/gic_v3_its.h> >> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c >> index 9c7e8f441c..0460962f08 100644 >> --- a/xen/arch/arm/vtimer.c >> +++ b/xen/arch/arm/vtimer.c >> @@ -22,6 +22,7 @@ >> #include <xen/sched.h> >> #include <xen/timer.h> >> >> +#include <asm/cpregs.h> >> #include <asm/div64.h> >> #include <asm/gic.h> >> #include <asm/irq.h> >> @@ -29,6 +30,7 @@ >> #include <asm/time.h> >> #include <asm/vgic.h> >> #include <asm/vreg.h> >> +#include <asm/regs.h> >> >> /* >> * Check if regs is allowed access, user_gate is tail end of a >> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h >> index 68cc82147e..fb330812af 100644 >> --- a/xen/include/asm-arm/arm32/processor.h >> +++ b/xen/include/asm-arm/arm32/processor.h >> @@ -1,6 +1,8 @@ >> #ifndef __ASM_ARM_ARM32_PROCESSOR_H >> #define __ASM_ARM_ARM32_PROCESSOR_H >> >> +#include <asm/cpregs.h> >> + >> #define ACTLR_CAXX_SMP (1<<6) >> >> #ifndef __ASSEMBLY__ >> diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h >> index 7968532462..cdf64e0f77 100644 >> --- a/xen/include/asm-arm/percpu.h >> +++ b/xen/include/asm-arm/percpu.h >> @@ -4,7 +4,6 @@ >> #ifndef __ASSEMBLY__ >> >> #include <xen/types.h> >> -#include <asm/cpregs.h> >> #if defined(CONFIG_ARM_32) >> # include <asm/arm32/processor.h> >> #elif defined(CONFIG_ARM_64) >> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h >> index d791c12c9c..cd45e5f48f 100644 >> --- a/xen/include/asm-arm/processor.h >> +++ b/xen/include/asm-arm/processor.h >> @@ -1,7 +1,6 @@ >> #ifndef __ASM_ARM_PROCESSOR_H >> #define __ASM_ARM_PROCESSOR_H >> >> -#include <asm/cpregs.h> >> #ifndef __ASSEMBLY__ >> #include <xen/types.h> >> #endif >> -- >> 2.11.0 >>
On Wed, 13 Sep 2017, Julien Grall wrote: > Hi Stefano, > > On 09/12/2017 10:53 PM, Stefano Stabellini wrote: > > On Tue, 12 Sep 2017, Julien Grall wrote: > > > Currently, cpregs.h is included in pretty much every files even for > > > arm64. However, the only use for arm64 is when emulating co-processors. > > > > > > For arm32, all users of processor.h expect cpregs.h to be included in > > > order to access co-processors. So move the inclusion in > > > asm-arm/arm32/processor.h. > > > > > > cpregs.h will also be directly included in the co-processors emulation > > > to accommodate arm64. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > I can see that the patch works and does what you describe, but what is > > the benefit? OK, we remove #include <asm/cpregs.h> from > > asm-arm/processor.h, but then we have to add it to vcpreg.c, vgic-v3.c, > > vtimer.c, and arm32/processor.h. Is there a long term benefit? What > > prompted you into writing this patch? > > asm/processor.h is included indirectly by every single file of the source > code. It seems that we use it as a placeholder for anything rather than > properly splitting in different headers. For instance it contains all the > definitions of the registers, the traps vector, SMC call, traps prototype... > For most of those stuff only a limited of files really care about it. So we > are polluting the name space of each file for no real benefits. > > On AArch64, cpregs.h is only useful when emulating co-processor gregisters. So > there are no point to include it in a main header that will be pulled by 100s > files just for the benetifs of 4 files. > > This patch is only a first attempt of clean-up processor.h. Ideally we should > have more patch to split it. All right. Acked-by: Stefano Stabellini <sstabellini@kernel.org> Maybe add a couple of lines to the commit description saying that by removing the inclusion of cpregs.h from asm/processor.h we drastically reduce the exposure of cpregs.h to any source files on ARM64. > > > --- > > > Changes in v2: > > > - Update commit message > > > --- > > > xen/arch/arm/smp.c | 1 - > > > xen/arch/arm/vcpreg.c | 1 + > > > xen/arch/arm/vgic-v3.c | 1 + > > > xen/arch/arm/vtimer.c | 2 ++ > > > xen/include/asm-arm/arm32/processor.h | 2 ++ > > > xen/include/asm-arm/percpu.h | 1 - > > > xen/include/asm-arm/processor.h | 1 - > > > 7 files changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c > > > index e7df0874d6..554f4992e6 100644 > > > --- a/xen/arch/arm/smp.c > > > +++ b/xen/arch/arm/smp.c > > > @@ -1,6 +1,5 @@ > > > #include <asm/system.h> > > > #include <asm/smp.h> > > > -#include <asm/cpregs.h> > > > #include <asm/page.h> > > > #include <asm/gic.h> > > > #include <asm/flushtlb.h> > > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > > > index f3b08403fb..e363183ba8 100644 > > > --- a/xen/arch/arm/vcpreg.c > > > +++ b/xen/arch/arm/vcpreg.c > > > @@ -18,6 +18,7 @@ > > > #include <xen/sched.h> > > > +#include <asm/cpregs.h> > > > #include <asm/current.h> > > > #include <asm/regs.h> > > > #include <asm/traps.h> > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > > index cbeac28b28..a0cf993d13 100644 > > > --- a/xen/arch/arm/vgic-v3.c > > > +++ b/xen/arch/arm/vgic-v3.c > > > @@ -26,6 +26,7 @@ > > > #include <xen/softirq.h> > > > #include <xen/sizes.h> > > > +#include <asm/cpregs.h> > > > #include <asm/current.h> > > > #include <asm/gic_v3_defs.h> > > > #include <asm/gic_v3_its.h> > > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > > > index 9c7e8f441c..0460962f08 100644 > > > --- a/xen/arch/arm/vtimer.c > > > +++ b/xen/arch/arm/vtimer.c > > > @@ -22,6 +22,7 @@ > > > #include <xen/sched.h> > > > #include <xen/timer.h> > > > +#include <asm/cpregs.h> > > > #include <asm/div64.h> > > > #include <asm/gic.h> > > > #include <asm/irq.h> > > > @@ -29,6 +30,7 @@ > > > #include <asm/time.h> > > > #include <asm/vgic.h> > > > #include <asm/vreg.h> > > > +#include <asm/regs.h> > > > /* > > > * Check if regs is allowed access, user_gate is tail end of a > > > diff --git a/xen/include/asm-arm/arm32/processor.h > > > b/xen/include/asm-arm/arm32/processor.h > > > index 68cc82147e..fb330812af 100644 > > > --- a/xen/include/asm-arm/arm32/processor.h > > > +++ b/xen/include/asm-arm/arm32/processor.h > > > @@ -1,6 +1,8 @@ > > > #ifndef __ASM_ARM_ARM32_PROCESSOR_H > > > #define __ASM_ARM_ARM32_PROCESSOR_H > > > +#include <asm/cpregs.h> > > > + > > > #define ACTLR_CAXX_SMP (1<<6) > > > #ifndef __ASSEMBLY__ > > > diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h > > > index 7968532462..cdf64e0f77 100644 > > > --- a/xen/include/asm-arm/percpu.h > > > +++ b/xen/include/asm-arm/percpu.h > > > @@ -4,7 +4,6 @@ > > > #ifndef __ASSEMBLY__ > > > #include <xen/types.h> > > > -#include <asm/cpregs.h> > > > #if defined(CONFIG_ARM_32) > > > # include <asm/arm32/processor.h> > > > #elif defined(CONFIG_ARM_64) > > > diff --git a/xen/include/asm-arm/processor.h > > > b/xen/include/asm-arm/processor.h > > > index d791c12c9c..cd45e5f48f 100644 > > > --- a/xen/include/asm-arm/processor.h > > > +++ b/xen/include/asm-arm/processor.h > > > @@ -1,7 +1,6 @@ > > > #ifndef __ASM_ARM_PROCESSOR_H > > > #define __ASM_ARM_PROCESSOR_H > > > -#include <asm/cpregs.h> > > > #ifndef __ASSEMBLY__ > > > #include <xen/types.h> > > > #endif > > > -- > > > 2.11.0 > > > > > -- > Julien Grall >
Hi Stefano, On 13/09/17 22:13, Stefano Stabellini wrote: > On Wed, 13 Sep 2017, Julien Grall wrote: >> Hi Stefano, >> >> On 09/12/2017 10:53 PM, Stefano Stabellini wrote: >>> On Tue, 12 Sep 2017, Julien Grall wrote: >>>> Currently, cpregs.h is included in pretty much every files even for >>>> arm64. However, the only use for arm64 is when emulating co-processors. >>>> >>>> For arm32, all users of processor.h expect cpregs.h to be included in >>>> order to access co-processors. So move the inclusion in >>>> asm-arm/arm32/processor.h. >>>> >>>> cpregs.h will also be directly included in the co-processors emulation >>>> to accommodate arm64. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> I can see that the patch works and does what you describe, but what is >>> the benefit? OK, we remove #include <asm/cpregs.h> from >>> asm-arm/processor.h, but then we have to add it to vcpreg.c, vgic-v3.c, >>> vtimer.c, and arm32/processor.h. Is there a long term benefit? What >>> prompted you into writing this patch? >> >> asm/processor.h is included indirectly by every single file of the source >> code. It seems that we use it as a placeholder for anything rather than >> properly splitting in different headers. For instance it contains all the >> definitions of the registers, the traps vector, SMC call, traps prototype... >> For most of those stuff only a limited of files really care about it. So we >> are polluting the name space of each file for no real benefits. >> >> On AArch64, cpregs.h is only useful when emulating co-processor gregisters. So >> there are no point to include it in a main header that will be pulled by 100s >> files just for the benetifs of 4 files. >> >> This patch is only a first attempt of clean-up processor.h. Ideally we should >> have more patch to split it. > > All right. > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > > Maybe add a couple of lines to the commit description saying that by > removing the inclusion of cpregs.h from asm/processor.h we drastically > reduce the exposure of cpregs.h to any source files on ARM64. I will do that. Cheers,
diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c index e7df0874d6..554f4992e6 100644 --- a/xen/arch/arm/smp.c +++ b/xen/arch/arm/smp.c @@ -1,6 +1,5 @@ #include <asm/system.h> #include <asm/smp.h> -#include <asm/cpregs.h> #include <asm/page.h> #include <asm/gic.h> #include <asm/flushtlb.h> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c index f3b08403fb..e363183ba8 100644 --- a/xen/arch/arm/vcpreg.c +++ b/xen/arch/arm/vcpreg.c @@ -18,6 +18,7 @@ #include <xen/sched.h> +#include <asm/cpregs.h> #include <asm/current.h> #include <asm/regs.h> #include <asm/traps.h> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index cbeac28b28..a0cf993d13 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -26,6 +26,7 @@ #include <xen/softirq.h> #include <xen/sizes.h> +#include <asm/cpregs.h> #include <asm/current.h> #include <asm/gic_v3_defs.h> #include <asm/gic_v3_its.h> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index 9c7e8f441c..0460962f08 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -22,6 +22,7 @@ #include <xen/sched.h> #include <xen/timer.h> +#include <asm/cpregs.h> #include <asm/div64.h> #include <asm/gic.h> #include <asm/irq.h> @@ -29,6 +30,7 @@ #include <asm/time.h> #include <asm/vgic.h> #include <asm/vreg.h> +#include <asm/regs.h> /* * Check if regs is allowed access, user_gate is tail end of a diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h index 68cc82147e..fb330812af 100644 --- a/xen/include/asm-arm/arm32/processor.h +++ b/xen/include/asm-arm/arm32/processor.h @@ -1,6 +1,8 @@ #ifndef __ASM_ARM_ARM32_PROCESSOR_H #define __ASM_ARM_ARM32_PROCESSOR_H +#include <asm/cpregs.h> + #define ACTLR_CAXX_SMP (1<<6) #ifndef __ASSEMBLY__ diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h index 7968532462..cdf64e0f77 100644 --- a/xen/include/asm-arm/percpu.h +++ b/xen/include/asm-arm/percpu.h @@ -4,7 +4,6 @@ #ifndef __ASSEMBLY__ #include <xen/types.h> -#include <asm/cpregs.h> #if defined(CONFIG_ARM_32) # include <asm/arm32/processor.h> #elif defined(CONFIG_ARM_64) diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index d791c12c9c..cd45e5f48f 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -1,7 +1,6 @@ #ifndef __ASM_ARM_PROCESSOR_H #define __ASM_ARM_PROCESSOR_H -#include <asm/cpregs.h> #ifndef __ASSEMBLY__ #include <xen/types.h> #endif
Currently, cpregs.h is included in pretty much every files even for arm64. However, the only use for arm64 is when emulating co-processors. For arm32, all users of processor.h expect cpregs.h to be included in order to access co-processors. So move the inclusion in asm-arm/arm32/processor.h. cpregs.h will also be directly included in the co-processors emulation to accommodate arm64. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Update commit message --- xen/arch/arm/smp.c | 1 - xen/arch/arm/vcpreg.c | 1 + xen/arch/arm/vgic-v3.c | 1 + xen/arch/arm/vtimer.c | 2 ++ xen/include/asm-arm/arm32/processor.h | 2 ++ xen/include/asm-arm/percpu.h | 1 - xen/include/asm-arm/processor.h | 1 - 7 files changed, 6 insertions(+), 3 deletions(-)