From: Christoffer Dall <christoffer.dall@linaro.org> To: Vladimir Murzin <vladimir.murzin@arm.com> Cc: marc.zyngier@arm.com, andre.przywara@arm.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 5/7] ARM: move system register accessors to asm/cp15.h Date: Tue, 6 Sep 2016 18:34:34 +0200 [thread overview] Message-ID: <20160906163434.GC23592@cbox> (raw) In-Reply-To: <57CEBF1A.60506@arm.com> On Tue, Sep 06, 2016 at 02:05:30PM +0100, Vladimir Murzin wrote: > 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. Thanks for the explanation! So a simpler way, IMHO, to explain this is to simply say that both linux/irqchip/arm-gic.v3.h and arch/arm/include/asm/kvm_hyp.h define a macro called __ACCESS_CP15, which obviously creates a conflict. Then you could explain if these are just different implementations of the same thing, or if they are semantically different, and finally how your patch solves this problem. > > >> > >> 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__) > >> + > > > > 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. > Hmm, I guess you're right, your approach does make sense. Thanks, -Christoffer
WARNING: multiple messages have this Message-ID (diff)
From: christoffer.dall@linaro.org (Christoffer Dall) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 5/7] ARM: move system register accessors to asm/cp15.h Date: Tue, 6 Sep 2016 18:34:34 +0200 [thread overview] Message-ID: <20160906163434.GC23592@cbox> (raw) In-Reply-To: <57CEBF1A.60506@arm.com> On Tue, Sep 06, 2016 at 02:05:30PM +0100, Vladimir Murzin wrote: > 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. Thanks for the explanation! So a simpler way, IMHO, to explain this is to simply say that both linux/irqchip/arm-gic.v3.h and arch/arm/include/asm/kvm_hyp.h define a macro called __ACCESS_CP15, which obviously creates a conflict. Then you could explain if these are just different implementations of the same thing, or if they are semantically different, and finally how your patch solves this problem. > > >> > >> 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__) > >> + > > > > 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. > Hmm, I guess you're right, your approach does make sense. Thanks, -Christoffer
next prev parent reply other threads:[~2016-09-06 16:23 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-16 10:46 [PATCH v2 0/7] ARM: KVM: Support for vgic-v3 Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-08-16 10:46 ` [PATCH v2 1/7] arm64: KVM: Move GIC accessors to arch_gicv3.h Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:28 ` Christoffer Dall 2016-09-05 11:28 ` Christoffer Dall 2016-09-06 12:33 ` Vladimir Murzin 2016-09-06 12:33 ` Vladimir Murzin 2016-08-16 10:46 ` [PATCH v2 2/7] arm64: KVM: Move vgic-v3 save/restore to virt/kvm/arm/hyp Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-08-16 10:46 ` [PATCH v2 3/7] KVM: arm: vgic-new: improve compatibility with 32-bit Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:29 ` Christoffer Dall 2016-09-05 11:29 ` Christoffer Dall 2016-09-06 12:41 ` Vladimir Murzin 2016-09-06 12:41 ` Vladimir Murzin 2016-09-06 13:22 ` Christoffer Dall 2016-09-06 13:22 ` Christoffer Dall 2016-09-06 13:54 ` Vladimir Murzin 2016-09-06 13:54 ` Vladimir Murzin 2016-09-06 16:31 ` Christoffer Dall 2016-09-06 16:31 ` Christoffer Dall 2016-09-07 9:06 ` Vladimir Murzin 2016-09-07 9:06 ` Vladimir Murzin 2016-09-07 9:43 ` Christoffer Dall 2016-09-07 9:43 ` Christoffer Dall 2016-08-16 10:46 ` [PATCH v2 4/7] ARM: update MPIDR accessors macro Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:29 ` Christoffer Dall 2016-09-05 11:29 ` Christoffer Dall 2016-09-06 12:42 ` Vladimir Murzin 2016-09-06 12:42 ` Vladimir Murzin 2016-08-16 10:46 ` [PATCH v2 5/7] ARM: move system register accessors to asm/cp15.h Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:29 ` Christoffer Dall 2016-09-05 11:29 ` Christoffer Dall 2016-09-06 13:05 ` Vladimir Murzin 2016-09-06 13:05 ` Vladimir Murzin 2016-09-06 16:34 ` Christoffer Dall [this message] 2016-09-06 16:34 ` Christoffer Dall 2016-08-16 10:46 ` [PATCH v2 6/7] ARM: KVM: Get ready to use vgic-v3 Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:29 ` Christoffer Dall 2016-09-05 11:29 ` Christoffer Dall 2016-09-06 13:12 ` Vladimir Murzin 2016-09-06 13:12 ` Vladimir Murzin 2016-09-06 16:49 ` Christoffer Dall 2016-09-06 16:49 ` Christoffer Dall 2016-08-16 10:46 ` [PATCH v2 7/7] ARM: KVM: Unlock vgic-v3 support Vladimir Murzin 2016-08-16 10:46 ` Vladimir Murzin 2016-09-05 11:29 ` Christoffer Dall 2016-09-05 11:29 ` Christoffer Dall 2016-09-06 13:08 ` Marc Zyngier 2016-09-06 13:08 ` Marc Zyngier 2016-09-06 13:18 ` Vladimir Murzin 2016-09-06 13:18 ` Vladimir Murzin 2016-09-06 16:52 ` Christoffer Dall 2016-09-06 16:52 ` Christoffer Dall 2016-09-06 13:23 ` Vladimir Murzin 2016-09-06 13:23 ` Vladimir Murzin 2016-09-06 16:55 ` Christoffer Dall 2016-09-06 16:55 ` Christoffer Dall 2016-09-07 10:48 ` Vladimir Murzin 2016-09-07 10:48 ` Vladimir Murzin 2016-09-07 12:58 ` Christoffer Dall 2016-09-07 12:58 ` Christoffer Dall 2016-09-07 14:20 ` Peter Maydell 2016-09-07 14:20 ` Peter Maydell 2016-09-07 14:47 ` Christoffer Dall 2016-09-07 14:47 ` Christoffer Dall 2016-09-05 11:28 ` [PATCH v2 0/7] ARM: KVM: Support for vgic-v3 Christoffer Dall 2016-09-05 11:28 ` Christoffer Dall 2016-09-06 12:32 ` Vladimir Murzin 2016-09-06 12:32 ` Vladimir Murzin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160906163434.GC23592@cbox \ --to=christoffer.dall@linaro.org \ --cc=andre.przywara@arm.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=marc.zyngier@arm.com \ --cc=vladimir.murzin@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.