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

[Julien Grall <julien.grall@arm.com>]

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
>>

-- 
Julien Grall

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