Re: [PATCH v3 7/8] ARM: Move system register accessors to asm/cp15.h

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 09/09/16 18:05, Marc Zyngier wrote:
> On 08/09/16 17:06, Vladimir Murzin wrote:
>> Headers linux/irqchip/arm-gic.v3.h and arch/arm/include/asm/kvm_hyp.h
>> are included in virt/kvm/arm/hyp/vgic-v3-sr.c and both define macros
>> called __ACCESS_CP15 and __ACCESS_CP15_64 which obviously creates a
>> conflict. These macros were introduced independently for GIC and KVM
>> and, in fact, do the same thing.
>>
>> As an option we could add prefixes to KVM and GIC version of macros so
>> they won't clash, but it'd introduce code duplication.  Alternatively,
>> we could keep macro in, say, GIC header and include it in KVM one (or
>> vice versa), but such dependency would not look nicer.
>>
>> So we follow arm64 way (it handles this via sysreg.h) and move only
>> single set of macros to asm/cp15.h
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@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__)
>> +
> 
> Shouldn't that be placed after the #ifdef below?
> 
>>  #ifdef CONFIG_CPU_CP15
>>  
>>  extern unsigned long cr_alignment;	/* defined in entry-armv.S */
>> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
>> index bd9434e..0b475d2 100644
>> --- a/arch/arm/include/asm/kvm_hyp.h
>> +++ b/arch/arm/include/asm/kvm_hyp.h
>> @@ -20,26 +20,13 @@
>>  
>>  #include <linux/compiler.h>
>>  #include <linux/kvm_host.h>
>> +#include <asm/cp15.h>
>>  #include <asm/kvm_mmu.h>
>>  #include <asm/vfp.h>
>>  
>> -#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 __ACCESS_VFP(CRn)			\
>>  	"mrc", "mcr", __stringify(p10, 7, %0, CRn, cr0, 0), u32
>>  
>> -#define __write_sysreg(v, r, w, c, t)	asm volatile(w " " c : : "r" ((t)(v)))
>> -#define write_sysreg(v, ...)		__write_sysreg(v, __VA_ARGS__)
>> -
>> -#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_special(v, r)					\
>>  	asm volatile("msr " __stringify(r) ", %0" : : "r" (v))
>>  #define read_special(r) ({					\
>>
> 
> Could you please cc RMK on this, given that this touches a core arch/arm
> file?

Ok. I'll cc/to him with the next re-spin.

Cheers
Vladimir

> 
> Thanks,
> 
> 	M.
>