[PATCH v2 5/7] ARM: move system register accessors to asm/cp15.h

[Vladimir Murzin]

On 05/09/16 12:29, Christoffer Dall wrote:
> On Tue, Aug 16, 2016 at 11:46:56AM +0100, Vladimir Murzin wrote:
>> Macro __ACCESS_CP15{_64} is defined in two headers (arch_gicv3.h and
>> kvm_hyp.h) which are going to be requested by vgic-v3 altogether.
>> GCC would not like it because it'd see that macro is redefined and (hey!)
>> they are different.  So, let's put only single macro version under common
>> place and use it everywhere.
> 
> I'm sorry, but I don't understand this commit text.
> 

Sorry for that :(

The issue the patch is trying to solve happens because
virt/kvm/arm/hyp/vgic-v3-sr.c has

#include <linux/irqchip/arm-gic-v3.h>
...
#include <asm/kvm_hyp.h>

each of these headers defines it's own __ACCESS_CP15 macro:

asm/kvm_hyp.h

#define __ACCESS_CP15(CRn, Op1, CRm, Op2)	\
	"mrc", "mcr", __stringify(p15, Op1, %0, CRn, CRm, Op2), u32

and linux/irqchip/arm-gic-v3.h via asm/arch_gicv3.h

#define __ACCESS_CP15(CRn, Op1, CRm, Op2) p15, Op1, %0, CRn, CRm, Op2

When these headers are used together conflict happens. The same applies
to __ACCESS_CP15_64 macro.

To address that only single set of macros is used and call sites updated.

>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin at arm.com>
>> ---
>>  arch/arm/include/asm/arch_gicv3.h |   27 +++++++++++----------------
>>  arch/arm/include/asm/cp15.h       |   15 +++++++++++++++
>>  arch/arm/include/asm/kvm_hyp.h    |   15 +--------------
>>  3 files changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
>> index e08d151..af25c32 100644
>> --- a/arch/arm/include/asm/arch_gicv3.h
>> +++ b/arch/arm/include/asm/arch_gicv3.h
>> @@ -22,9 +22,7 @@
>>  
>>  #include <linux/io.h>
>>  #include <asm/barrier.h>
>> -
>> -#define __ACCESS_CP15(CRn, Op1, CRm, Op2)	p15, Op1, %0, CRn, CRm, Op2
>> -#define __ACCESS_CP15_64(Op1, CRm)		p15, Op1, %Q0, %R0, CRm
>> +#include <asm/cp15.h>
>>  
>>  #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
>>  #define ICC_DIR				__ACCESS_CP15(c12, 0, c11, 1)
>> @@ -102,58 +100,55 @@
>>  
>>  static inline void gic_write_eoir(u32 irq)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_EOIR1) : : "r" (irq));
>> +	write_sysreg(irq, ICC_EOIR1);
>>  	isb();
>>  }
>>  
>>  static inline void gic_write_dir(u32 val)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_DIR) : : "r" (val));
>> +	write_sysreg(val, ICC_DIR);
>>  	isb();
>>  }
>>  
>>  static inline u32 gic_read_iar(void)
>>  {
>> -	u32 irqstat;
>> +	u32 irqstat = read_sysreg(ICC_IAR1);
>>  
>> -	asm volatile("mrc " __stringify(ICC_IAR1) : "=r" (irqstat));
>>  	dsb(sy);
>> +
>>  	return irqstat;
>>  }
>>  
>>  static inline void gic_write_pmr(u32 val)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_PMR) : : "r" (val));
>> +	write_sysreg(val, ICC_PMR);
>>  }
>>  
>>  static inline void gic_write_ctlr(u32 val)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_CTLR) : : "r" (val));
>> +	write_sysreg(val, ICC_CTLR);
>>  	isb();
>>  }
>>  
>>  static inline void gic_write_grpen1(u32 val)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_IGRPEN1) : : "r" (val));
>> +	write_sysreg(val, ICC_IGRPEN1);
>>  	isb();
>>  }
>>  
>>  static inline void gic_write_sgi1r(u64 val)
>>  {
>> -	asm volatile("mcrr " __stringify(ICC_SGI1R) : : "r" (val));
>> +	write_sysreg(val, ICC_SGI1R);
>>  }
>>  
>>  static inline u32 gic_read_sre(void)
>>  {
>> -	u32 val;
>> -
>> -	asm volatile("mrc " __stringify(ICC_SRE) : "=r" (val));
>> -	return val;
>> +	return read_sysreg(ICC_SRE);
>>  }
>>  
>>  static inline void gic_write_sre(u32 val)
>>  {
>> -	asm volatile("mcr " __stringify(ICC_SRE) : : "r" (val));
>> +	write_sysreg(val, ICC_SRE);
>>  	isb();
>>  }
>>  
>> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
>> index c3f1152..f661732 100644
>> --- a/arch/arm/include/asm/cp15.h
>> +++ b/arch/arm/include/asm/cp15.h
>> @@ -47,6 +47,21 @@
>>  #define vectors_high()	(0)
>>  #endif
>>  
>> +#define __ACCESS_CP15(CRn, Op1, CRm, Op2)	\
>> +	"mrc", "mcr", __stringify(p15, Op1, %0, CRn, CRm, Op2), u32
>> +#define __ACCESS_CP15_64(Op1, CRm)		\
>> +	"mrrc", "mcrr", __stringify(p15, Op1, %Q0, %R0, CRm), u64
>> +
>> +#define __read_sysreg(r, w, c, t) ({				\
>> +	t __val;						\
>> +	asm volatile(r " " c : "=r" (__val));			\
>> +	__val;							\
>> +})
>> +#define read_sysreg(...)		__read_sysreg(__VA_ARGS__)
>> +
>> +#define __write_sysreg(v, r, w, c, t)	asm volatile(w " " c : : "r" ((t)(v)))
>> +#define write_sysreg(v, ...)		__write_sysreg(v, __VA_ARGS__)
>> +
> 
> I feel a bit strange about adding this sort of stuff in a
> non-kvm-non-gic-specific ARM header file, without it being used (or
> planned to be used) in a broader sense.
> 
> Is there not a way to keep the required changes local to KVM and the
> gic?
> 

We could add prefixes to KVM and GIC version of macros so they won't
clash, but it'd introduce code duplication.
We could keep macro in, say, GIC header and include it in KVM one (or
vice versa), but such dependency would not look nicer, IMO.

The way arm64 handles this is via sysreg.h and the closest counterpart
under arch/arm is cp15.h

I'm open to suggestions.

Thanks
Vladimir