Re: [Xen-devel] [PATCH v2 7/7] xen/arm: Limit the scope of cpregs.h

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> > 
> > 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@xxxxxxxxxx>

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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel