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

Wed, 13 Sep 2017 02:12:08 -0700 

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.gr...@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



--
Julien Grall

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

Reply via email to