linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
@ 2014-01-08 18:00 Stefano Stabellini
  2014-01-09 10:30 ` Will Deacon
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2014-01-08 18:00 UTC (permalink / raw)
  To: linux
  Cc: stefano.stabellini, arnd, will.deacon, gang.chen,
	catalin.marinas, jaccon.bastiaansen, linux-arm-kernel,
	linux-kernel

Remove !GENERIC_ATOMIC64 build dependency:
- rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
  GENERIC_ATOMIC64;
- call armv7_atomic64_xchg directly from xen/events.h.

Remove !CPU_V6 build dependency:
- introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
  CONFIG_CPU_V6;
- implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: arnd@arndb.de
CC: linux@arm.linux.org.uk
CC: will.deacon@arm.com
CC: gang.chen@asianux.com
CC: catalin.marinas@arm.com
CC: jaccon.bastiaansen@gmail.com
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org


---

Changes in v3:
- rebase.

Changes in v2:
- handle return value of __cmpxchg8 and __cmpxchg16 in __cmpxchg;
- remove sync_cmpxchg to sync_cmpxchg16 rename in grant-table.c,
  implement a generic sync_cmpxchg instead.
---
 arch/arm/Kconfig                   |    3 +-
 arch/arm/include/asm/atomic.h      |   47 +++++++++++++++-------------
 arch/arm/include/asm/cmpxchg.h     |   60 ++++++++++++++++++++++++------------
 arch/arm/include/asm/sync_bitops.h |   24 ++++++++++++++-
 arch/arm/include/asm/xen/events.h  |    2 +-
 5 files changed, 91 insertions(+), 45 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c1f1a7e..ae54ae0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1881,8 +1881,7 @@ config XEN_DOM0
 config XEN
 	bool "Xen guest support on ARM (EXPERIMENTAL)"
 	depends on ARM && AEABI && OF
-	depends on CPU_V7 && !CPU_V6
-	depends on !GENERIC_ATOMIC64
+	depends on CPU_V7
 	select ARM_PSCI
 	select SWIOTLB_XEN
 	help
diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 62d2cb5..dfe1cd6 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -382,26 +382,7 @@ static inline long long atomic64_cmpxchg(atomic64_t *ptr, long long old,
 	return oldval;
 }
 
-static inline long long atomic64_xchg(atomic64_t *ptr, long long new)
-{
-	long long result;
-	unsigned long tmp;
-
-	smp_mb();
-
-	__asm__ __volatile__("@ atomic64_xchg\n"
-"1:	ldrexd	%0, %H0, [%3]\n"
-"	strexd	%1, %4, %H4, [%3]\n"
-"	teq	%1, #0\n"
-"	bne	1b"
-	: "=&r" (result), "=&r" (tmp), "+Qo" (ptr->counter)
-	: "r" (&ptr->counter), "r" (new)
-	: "cc");
-
-	smp_mb();
-
-	return result;
-}
+#define armv7_atomic64_xchg atomic64_xchg
 
 static inline long long atomic64_dec_if_positive(atomic64_t *v)
 {
@@ -469,6 +450,30 @@ static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)
 #define atomic64_dec_and_test(v)	(atomic64_dec_return((v)) == 0)
 #define atomic64_inc_not_zero(v)	atomic64_add_unless((v), 1LL, 0LL)
 
-#endif /* !CONFIG_GENERIC_ATOMIC64 */
+#else /* !CONFIG_GENERIC_ATOMIC64 */
+#include <asm-generic/atomic64.h>
+#endif
+
+static inline u64 armv7_atomic64_xchg(atomic64_t *ptr, u64 new)
+{
+	long long result;
+	unsigned long tmp;
+
+	smp_mb();
+
+	__asm__ __volatile__("@ armv7_atomic64_xchg\n"
+"1:	ldrexd	%0, %H0, [%3]\n"
+"	strexd	%1, %4, %H4, [%3]\n"
+"	teq	%1, #0\n"
+"	bne	1b"
+	: "=&r" (result), "=&r" (tmp), "+Qo" (ptr->counter)
+	: "r" (&ptr->counter), "r" (new)
+	: "cc");
+
+	smp_mb();
+
+	return result;
+}
+
 #endif
 #endif
diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h
index df2fbba..cc8a4a2 100644
--- a/arch/arm/include/asm/cmpxchg.h
+++ b/arch/arm/include/asm/cmpxchg.h
@@ -133,6 +133,44 @@ extern void __bad_cmpxchg(volatile void *ptr, int size);
  * cmpxchg only support 32-bits operands on ARMv6.
  */
 
+static inline unsigned long __cmpxchg8(volatile void *ptr, unsigned long old,
+				      unsigned long new)
+{
+	unsigned long oldval, res;
+
+	do {
+		asm volatile("@ __cmpxchg1\n"
+		"	ldrexb	%1, [%2]\n"
+		"	mov	%0, #0\n"
+		"	teq	%1, %3\n"
+		"	strexbeq %0, %4, [%2]\n"
+			: "=&r" (res), "=&r" (oldval)
+			: "r" (ptr), "Ir" (old), "r" (new)
+			: "memory", "cc");
+	} while (res);
+
+	return oldval;
+}
+
+static inline unsigned long __cmpxchg16(volatile void *ptr, unsigned long old,
+				      unsigned long new)
+{
+	unsigned long oldval, res;
+
+	do {
+		asm volatile("@ __cmpxchg1\n"
+		"	ldrexh	%1, [%2]\n"
+		"	mov	%0, #0\n"
+		"	teq	%1, %3\n"
+		"	strexheq %0, %4, [%2]\n"
+			: "=&r" (res), "=&r" (oldval)
+			: "r" (ptr), "Ir" (old), "r" (new)
+			: "memory", "cc");
+	} while (res);
+
+	return oldval;
+}
+
 static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 				      unsigned long new, int size)
 {
@@ -141,28 +179,10 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 	switch (size) {
 #ifndef CONFIG_CPU_V6	/* min ARCH >= ARMv6K */
 	case 1:
-		do {
-			asm volatile("@ __cmpxchg1\n"
-			"	ldrexb	%1, [%2]\n"
-			"	mov	%0, #0\n"
-			"	teq	%1, %3\n"
-			"	strexbeq %0, %4, [%2]\n"
-				: "=&r" (res), "=&r" (oldval)
-				: "r" (ptr), "Ir" (old), "r" (new)
-				: "memory", "cc");
-		} while (res);
+		oldval = __cmpxchg8(ptr, old, new);
 		break;
 	case 2:
-		do {
-			asm volatile("@ __cmpxchg1\n"
-			"	ldrexh	%1, [%2]\n"
-			"	mov	%0, #0\n"
-			"	teq	%1, %3\n"
-			"	strexheq %0, %4, [%2]\n"
-				: "=&r" (res), "=&r" (oldval)
-				: "r" (ptr), "Ir" (old), "r" (new)
-				: "memory", "cc");
-		} while (res);
+		oldval = __cmpxchg16(ptr, old, new);
 		break;
 #endif
 	case 4:
diff --git a/arch/arm/include/asm/sync_bitops.h b/arch/arm/include/asm/sync_bitops.h
index 63479ee..942659a 100644
--- a/arch/arm/include/asm/sync_bitops.h
+++ b/arch/arm/include/asm/sync_bitops.h
@@ -21,7 +21,29 @@
 #define sync_test_and_clear_bit(nr, p)	_test_and_clear_bit(nr, p)
 #define sync_test_and_change_bit(nr, p)	_test_and_change_bit(nr, p)
 #define sync_test_bit(nr, addr)		test_bit(nr, addr)
-#define sync_cmpxchg			cmpxchg
 
+static inline unsigned long sync_cmpxchg(volatile void *ptr,
+										 unsigned long old,
+										 unsigned long new)
+{
+	unsigned long oldval;
+	int size = sizeof(*(ptr));
+
+	smp_mb();
+	switch (size) {
+	case 1:
+		oldval = __cmpxchg8(ptr, old, new);
+		break;
+	case 2:
+		oldval = __cmpxchg16(ptr, old, new);
+		break;
+	default:
+		oldval = __cmpxchg(ptr, old, new, size);
+		break;
+	}
+	smp_mb();
+
+	return oldval;
+}
 
 #endif
diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
index 8b1f37b..4269519 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -16,7 +16,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
 	return raw_irqs_disabled_flags(regs->ARM_cpsr);
 }
 
-#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr),	\
+#define xchg_xen_ulong(ptr, val) armv7_atomic64_xchg(container_of((ptr),	\
 							    atomic64_t,	\
 							    counter), (val))
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-08 18:00 [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN Stefano Stabellini
@ 2014-01-09 10:30 ` Will Deacon
  2014-01-09 11:04   ` Arnd Bergmann
  0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2014-01-09 10:30 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: linux, arnd, gang.chen, Catalin Marinas, jaccon.bastiaansen,
	linux-arm-kernel, linux-kernel

Hi Stefano,

On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
> Remove !GENERIC_ATOMIC64 build dependency:
> - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
>   GENERIC_ATOMIC64;
> - call armv7_atomic64_xchg directly from xen/events.h.
> 
> Remove !CPU_V6 build dependency:
> - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
>   CONFIG_CPU_V6;
> - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: arnd@arndb.de
> CC: linux@arm.linux.org.uk
> CC: will.deacon@arm.com
> CC: gang.chen@asianux.com
> CC: catalin.marinas@arm.com
> CC: jaccon.bastiaansen@gmail.com
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> 

I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
What am I missing?

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-09 10:30 ` Will Deacon
@ 2014-01-09 11:04   ` Arnd Bergmann
  2014-01-09 12:47     ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Arnd Bergmann @ 2014-01-09 11:04 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Will Deacon, Stefano Stabellini, linux, Catalin Marinas,
	gang.chen, linux-kernel, jaccon.bastiaansen

On Thursday 09 January 2014, Will Deacon wrote:
> On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
> > Remove !GENERIC_ATOMIC64 build dependency:
> > - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
> >   GENERIC_ATOMIC64;
> > - call armv7_atomic64_xchg directly from xen/events.h.
> > 
> > Remove !CPU_V6 build dependency:
> > - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
> >   CONFIG_CPU_V6;
> > - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: arnd@arndb.de
> > CC: linux@arm.linux.org.uk
> > CC: will.deacon@arm.com
> > CC: gang.chen@asianux.com
> > CC: catalin.marinas@arm.com
> > CC: jaccon.bastiaansen@gmail.com
> > CC: linux-arm-kernel@lists.infradead.org
> > CC: linux-kernel@vger.kernel.org
> > 
> 
> I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
> What am I missing?

This is about being able to build a kernel that runs on ARMv6 and ARMv7
and also includes Xen. Because of obvious hardware limitations, Xen
will only run on v7, but currently you cannot even build it once you
enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
do atomic accesses in a generic way on non-32bit variables.

	Arnd

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-09 11:04   ` Arnd Bergmann
@ 2014-01-09 12:47     ` Stefano Stabellini
  2014-01-09 18:42       ` Will Deacon
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2014-01-09 12:47 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Will Deacon, Stefano Stabellini, linux,
	Catalin Marinas, gang.chen, linux-kernel, jaccon.bastiaansen

On Thu, 9 Jan 2014, Arnd Bergmann wrote:
> On Thursday 09 January 2014, Will Deacon wrote:
> > On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
> > > Remove !GENERIC_ATOMIC64 build dependency:
> > > - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
> > >   GENERIC_ATOMIC64;
> > > - call armv7_atomic64_xchg directly from xen/events.h.
> > > 
> > > Remove !CPU_V6 build dependency:
> > > - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
> > >   CONFIG_CPU_V6;
> > > - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > CC: arnd@arndb.de
> > > CC: linux@arm.linux.org.uk
> > > CC: will.deacon@arm.com
> > > CC: gang.chen@asianux.com
> > > CC: catalin.marinas@arm.com
> > > CC: jaccon.bastiaansen@gmail.com
> > > CC: linux-arm-kernel@lists.infradead.org
> > > CC: linux-kernel@vger.kernel.org
> > > 
> > 
> > I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
> > What am I missing?
> 
> This is about being able to build a kernel that runs on ARMv6 and ARMv7
> and also includes Xen. Because of obvious hardware limitations, Xen
> will only run on v7, but currently you cannot even build it once you
> enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
> do atomic accesses in a generic way on non-32bit variables.

Yep, that's right.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-09 12:47     ` Stefano Stabellini
@ 2014-01-09 18:42       ` Will Deacon
  2014-01-10 16:48         ` Chen Gang F T
  2014-01-16 16:27         ` Stefano Stabellini
  0 siblings, 2 replies; 12+ messages in thread
From: Will Deacon @ 2014-01-09 18:42 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Arnd Bergmann, linux-arm-kernel, linux, Catalin Marinas,
	gang.chen, linux-kernel, jaccon.bastiaansen

On Thu, Jan 09, 2014 at 12:47:24PM +0000, Stefano Stabellini wrote:
> On Thu, 9 Jan 2014, Arnd Bergmann wrote:
> > On Thursday 09 January 2014, Will Deacon wrote:
> > > On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
> > > > Remove !GENERIC_ATOMIC64 build dependency:
> > > > - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
> > > >   GENERIC_ATOMIC64;
> > > > - call armv7_atomic64_xchg directly from xen/events.h.
> > > > 
> > > > Remove !CPU_V6 build dependency:
> > > > - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
> > > >   CONFIG_CPU_V6;
> > > > - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > CC: arnd@arndb.de
> > > > CC: linux@arm.linux.org.uk
> > > > CC: will.deacon@arm.com
> > > > CC: gang.chen@asianux.com
> > > > CC: catalin.marinas@arm.com
> > > > CC: jaccon.bastiaansen@gmail.com
> > > > CC: linux-arm-kernel@lists.infradead.org
> > > > CC: linux-kernel@vger.kernel.org
> > > > 
> > > 
> > > I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
> > > What am I missing?
> > 
> > This is about being able to build a kernel that runs on ARMv6 and ARMv7
> > and also includes Xen. Because of obvious hardware limitations, Xen
> > will only run on v7, but currently you cannot even build it once you
> > enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
> > do atomic accesses in a generic way on non-32bit variables.
> 
> Yep, that's right.

Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
not cleaner just to implement xchg code separately for Xen? The Linux code
isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
churn coming out of this patch is an attempt to provide some small code
reuse at the cost of code readability.

What do others think?

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-09 18:42       ` Will Deacon
@ 2014-01-10 16:48         ` Chen Gang F T
  2014-01-13  8:12           ` Jaccon Bastiaansen
  2014-01-16 16:27         ` Stefano Stabellini
  1 sibling, 1 reply; 12+ messages in thread
From: Chen Gang F T @ 2014-01-10 16:48 UTC (permalink / raw)
  To: Will Deacon
  Cc: Stefano Stabellini, Arnd Bergmann, linux-arm-kernel, linux,
	Catalin Marinas, gang.chen, linux-kernel, jaccon.bastiaansen

On 01/10/2014 02:42 AM, Will Deacon wrote:
> On Thu, Jan 09, 2014 at 12:47:24PM +0000, Stefano Stabellini wrote:
>> On Thu, 9 Jan 2014, Arnd Bergmann wrote:
>>> On Thursday 09 January 2014, Will Deacon wrote:
>>>> On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
>>>>> Remove !GENERIC_ATOMIC64 build dependency:
>>>>> - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
>>>>>   GENERIC_ATOMIC64;
>>>>> - call armv7_atomic64_xchg directly from xen/events.h.
>>>>>
>>>>> Remove !CPU_V6 build dependency:
>>>>> - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
>>>>>   CONFIG_CPU_V6;
>>>>> - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
>>>>>
>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>> CC: arnd@arndb.de
>>>>> CC: linux@arm.linux.org.uk
>>>>> CC: will.deacon@arm.com
>>>>> CC: gang.chen@asianux.com
>>>>> CC: catalin.marinas@arm.com
>>>>> CC: jaccon.bastiaansen@gmail.com
>>>>> CC: linux-arm-kernel@lists.infradead.org
>>>>> CC: linux-kernel@vger.kernel.org
>>>>>
>>>>
>>>> I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
>>>> What am I missing?
>>>
>>> This is about being able to build a kernel that runs on ARMv6 and ARMv7
>>> and also includes Xen. Because of obvious hardware limitations, Xen
>>> will only run on v7, but currently you cannot even build it once you
>>> enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
>>> do atomic accesses in a generic way on non-32bit variables.
>>
>> Yep, that's right.
> 
> Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
> not cleaner just to implement xchg code separately for Xen? The Linux code
> isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
> churn coming out of this patch is an attempt to provide some small code
> reuse at the cost of code readability.
> 
> What do others think?
> 

What Will said sounds reasonable to me.


Thanks.
-- 
Chen Gang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-10 16:48         ` Chen Gang F T
@ 2014-01-13  8:12           ` Jaccon Bastiaansen
  0 siblings, 0 replies; 12+ messages in thread
From: Jaccon Bastiaansen @ 2014-01-13  8:12 UTC (permalink / raw)
  To: Chen Gang F T
  Cc: Will Deacon, Stefano Stabellini, Arnd Bergmann, linux-arm-kernel,
	linux, Catalin Marinas, gang.chen, linux-kernel

2014/1/10 Chen Gang F T <chen.gang.flying.transformer@gmail.com>:
> On 01/10/2014 02:42 AM, Will Deacon wrote:
>> On Thu, Jan 09, 2014 at 12:47:24PM +0000, Stefano Stabellini wrote:
>>> On Thu, 9 Jan 2014, Arnd Bergmann wrote:
>>>> On Thursday 09 January 2014, Will Deacon wrote:
>>>>> On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
>>>>>> Remove !GENERIC_ATOMIC64 build dependency:
>>>>>> - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
>>>>>>   GENERIC_ATOMIC64;
>>>>>> - call armv7_atomic64_xchg directly from xen/events.h.
>>>>>>
>>>>>> Remove !CPU_V6 build dependency:
>>>>>> - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
>>>>>>   CONFIG_CPU_V6;
>>>>>> - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
>>>>>>
>>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>>> CC: arnd@arndb.de
>>>>>> CC: linux@arm.linux.org.uk
>>>>>> CC: will.deacon@arm.com
>>>>>> CC: gang.chen@asianux.com
>>>>>> CC: catalin.marinas@arm.com
>>>>>> CC: jaccon.bastiaansen@gmail.com
>>>>>> CC: linux-arm-kernel@lists.infradead.org
>>>>>> CC: linux-kernel@vger.kernel.org
>>>>>>
>>>>>
>>>>> I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
>>>>> What am I missing?
>>>>
>>>> This is about being able to build a kernel that runs on ARMv6 and ARMv7
>>>> and also includes Xen. Because of obvious hardware limitations, Xen
>>>> will only run on v7, but currently you cannot even build it once you
>>>> enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
>>>> do atomic accesses in a generic way on non-32bit variables.
>>>
>>> Yep, that's right.
>>
>> Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
>> not cleaner just to implement xchg code separately for Xen? The Linux code
>> isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
>> churn coming out of this patch is an attempt to provide some small code
>> reuse at the cost of code readability.
>>
>> What do others think?
>>
>
> What Will said sounds reasonable to me.
>
>
> Thanks.
> --
> Chen Gang

I agree with Will,

Regards,
  Jaccon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-09 18:42       ` Will Deacon
  2014-01-10 16:48         ` Chen Gang F T
@ 2014-01-16 16:27         ` Stefano Stabellini
  2014-01-16 19:31           ` Will Deacon
  1 sibling, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2014-01-16 16:27 UTC (permalink / raw)
  To: Will Deacon
  Cc: Stefano Stabellini, Arnd Bergmann, linux-arm-kernel, linux,
	Catalin Marinas, gang.chen, linux-kernel, jaccon.bastiaansen

On Thu, 9 Jan 2014, Will Deacon wrote:
> On Thu, Jan 09, 2014 at 12:47:24PM +0000, Stefano Stabellini wrote:
> > On Thu, 9 Jan 2014, Arnd Bergmann wrote:
> > > On Thursday 09 January 2014, Will Deacon wrote:
> > > > On Wed, Jan 08, 2014 at 06:00:23PM +0000, Stefano Stabellini wrote:
> > > > > Remove !GENERIC_ATOMIC64 build dependency:
> > > > > - rename atomic64_xchg to armv7_atomic64_xchg and define it even ifdef
> > > > >   GENERIC_ATOMIC64;
> > > > > - call armv7_atomic64_xchg directly from xen/events.h.
> > > > > 
> > > > > Remove !CPU_V6 build dependency:
> > > > > - introduce __cmpxchg8 and __cmpxchg16, compiled even ifdef
> > > > >   CONFIG_CPU_V6;
> > > > > - implement sync_cmpxchg using __cmpxchg8 and __cmpxchg16.
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > > CC: arnd@arndb.de
> > > > > CC: linux@arm.linux.org.uk
> > > > > CC: will.deacon@arm.com
> > > > > CC: gang.chen@asianux.com
> > > > > CC: catalin.marinas@arm.com
> > > > > CC: jaccon.bastiaansen@gmail.com
> > > > > CC: linux-arm-kernel@lists.infradead.org
> > > > > CC: linux-kernel@vger.kernel.org
> > > > > 
> > > > 
> > > > I'm confused here. It looks like you want to call armv7 code in a v6 kernel.
> > > > What am I missing?
> > > 
> > > This is about being able to build a kernel that runs on ARMv6 and ARMv7
> > > and also includes Xen. Because of obvious hardware limitations, Xen
> > > will only run on v7, but currently you cannot even build it once you
> > > enable (pre-v6K) ARMv6 support, since the combined v6+v7 kernel can't
> > > do atomic accesses in a generic way on non-32bit variables.
> > 
> > Yep, that's right.
> 
> Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
> not cleaner just to implement xchg code separately for Xen? The Linux code
> isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
> churn coming out of this patch is an attempt to provide some small code
> reuse at the cost of code readability.
> 
> What do others think?

I am OK with that, in fact my first version of the patch did just that:

http://marc.info/?l=linux-arm-kernel&m=138436406724990&w=2

Is that what you had in mind?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-16 16:27         ` Stefano Stabellini
@ 2014-01-16 19:31           ` Will Deacon
  2014-01-20 15:32             ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2014-01-16 19:31 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Arnd Bergmann, linux-arm-kernel, linux, Catalin Marinas,
	gang.chen, linux-kernel, jaccon.bastiaansen

Hi Stefano,

On Thu, Jan 16, 2014 at 04:27:55PM +0000, Stefano Stabellini wrote:
> On Thu, 9 Jan 2014, Will Deacon wrote:
> > Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
> > not cleaner just to implement xchg code separately for Xen? The Linux code
> > isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
> > churn coming out of this patch is an attempt to provide some small code
> > reuse at the cost of code readability.
> > 
> > What do others think?
> 
> I am OK with that, in fact my first version of the patch did just that:
> 
> http://marc.info/?l=linux-arm-kernel&m=138436406724990&w=2
> 
> Is that what you had in mind?

For the xchg part, yes, that looks a lot better. I don't like the #undef
CONFIG_CPU_V6 though, can that be rewritten to use __LINUX_ARM_ARCH__?

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-16 19:31           ` Will Deacon
@ 2014-01-20 15:32             ` Stefano Stabellini
  2014-01-21 11:08               ` Will Deacon
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Stabellini @ 2014-01-20 15:32 UTC (permalink / raw)
  To: Will Deacon
  Cc: Stefano Stabellini, Arnd Bergmann, linux-arm-kernel, linux,
	Catalin Marinas, gang.chen, linux-kernel, jaccon.bastiaansen

On Thu, 16 Jan 2014, Will Deacon wrote:
> Hi Stefano,
> 
> On Thu, Jan 16, 2014 at 04:27:55PM +0000, Stefano Stabellini wrote:
> > On Thu, 9 Jan 2014, Will Deacon wrote:
> > > Ok, thanks for the explanation. Looking at the patch, I wonder whether it's
> > > not cleaner just to implement xchg code separately for Xen? The Linux code
> > > isn't always sufficient (due to the GENERIC_ATOMIC64 stuff) and most of the
> > > churn coming out of this patch is an attempt to provide some small code
> > > reuse at the cost of code readability.
> > > 
> > > What do others think?
> > 
> > I am OK with that, in fact my first version of the patch did just that:
> > 
> > http://marc.info/?l=linux-arm-kernel&m=138436406724990&w=2
> > 
> > Is that what you had in mind?
> 
> For the xchg part, yes, that looks a lot better. I don't like the #undef
> CONFIG_CPU_V6 though, can that be rewritten to use __LINUX_ARM_ARCH__?

The problem is that the 1 and 2 byte parameter size cases in __cmpxchg
are ifdef'ed CONFIG_CPU_V6 but drivers/xen/grant-table.c needs them.

So we can either undef CONFIG_CPU_V6 in grant-table.c or call a
different function.

If I switch from ifdef CONFIG_CPU_V6 to if __LINUX_ARM_ARCH__ > 6 in
__cmpxchg, we still have the problem that if __LINUX_ARM_ARCH__ == 6,
grant-table.c doesn't compile.

Maybe the approach taken by the other patch for cmpxchg is better, see
below.


diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c1f1a7e..ae54ae0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1881,8 +1881,7 @@ config XEN_DOM0
 config XEN
 	bool "Xen guest support on ARM (EXPERIMENTAL)"
 	depends on ARM && AEABI && OF
-	depends on CPU_V7 && !CPU_V6
-	depends on !GENERIC_ATOMIC64
+	depends on CPU_V7
 	select ARM_PSCI
 	select SWIOTLB_XEN
 	help
diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h
index df2fbba..cc8a4a2 100644
--- a/arch/arm/include/asm/cmpxchg.h
+++ b/arch/arm/include/asm/cmpxchg.h
@@ -133,6 +133,44 @@ extern void __bad_cmpxchg(volatile void *ptr, int size);
  * cmpxchg only support 32-bits operands on ARMv6.
  */
 
+static inline unsigned long __cmpxchg8(volatile void *ptr, unsigned long old,
+				      unsigned long new)
+{
+	unsigned long oldval, res;
+
+	do {
+		asm volatile("@ __cmpxchg1\n"
+		"	ldrexb	%1, [%2]\n"
+		"	mov	%0, #0\n"
+		"	teq	%1, %3\n"
+		"	strexbeq %0, %4, [%2]\n"
+			: "=&r" (res), "=&r" (oldval)
+			: "r" (ptr), "Ir" (old), "r" (new)
+			: "memory", "cc");
+	} while (res);
+
+	return oldval;
+}
+
+static inline unsigned long __cmpxchg16(volatile void *ptr, unsigned long old,
+				      unsigned long new)
+{
+	unsigned long oldval, res;
+
+	do {
+		asm volatile("@ __cmpxchg1\n"
+		"	ldrexh	%1, [%2]\n"
+		"	mov	%0, #0\n"
+		"	teq	%1, %3\n"
+		"	strexheq %0, %4, [%2]\n"
+			: "=&r" (res), "=&r" (oldval)
+			: "r" (ptr), "Ir" (old), "r" (new)
+			: "memory", "cc");
+	} while (res);
+
+	return oldval;
+}
+
 static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 				      unsigned long new, int size)
 {
@@ -141,28 +179,10 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 	switch (size) {
 #ifndef CONFIG_CPU_V6	/* min ARCH >= ARMv6K */
 	case 1:
-		do {
-			asm volatile("@ __cmpxchg1\n"
-			"	ldrexb	%1, [%2]\n"
-			"	mov	%0, #0\n"
-			"	teq	%1, %3\n"
-			"	strexbeq %0, %4, [%2]\n"
-				: "=&r" (res), "=&r" (oldval)
-				: "r" (ptr), "Ir" (old), "r" (new)
-				: "memory", "cc");
-		} while (res);
+		oldval = __cmpxchg8(ptr, old, new);
 		break;
 	case 2:
-		do {
-			asm volatile("@ __cmpxchg1\n"
-			"	ldrexh	%1, [%2]\n"
-			"	mov	%0, #0\n"
-			"	teq	%1, %3\n"
-			"	strexheq %0, %4, [%2]\n"
-				: "=&r" (res), "=&r" (oldval)
-				: "r" (ptr), "Ir" (old), "r" (new)
-				: "memory", "cc");
-		} while (res);
+		oldval = __cmpxchg16(ptr, old, new);
 		break;
 #endif
 	case 4:
diff --git a/arch/arm/include/asm/sync_bitops.h b/arch/arm/include/asm/sync_bitops.h
index 63479ee..942659a 100644
--- a/arch/arm/include/asm/sync_bitops.h
+++ b/arch/arm/include/asm/sync_bitops.h
@@ -21,7 +21,29 @@
 #define sync_test_and_clear_bit(nr, p)	_test_and_clear_bit(nr, p)
 #define sync_test_and_change_bit(nr, p)	_test_and_change_bit(nr, p)
 #define sync_test_bit(nr, addr)		test_bit(nr, addr)
-#define sync_cmpxchg			cmpxchg
 
+static inline unsigned long sync_cmpxchg(volatile void *ptr,
+										 unsigned long old,
+										 unsigned long new)
+{
+	unsigned long oldval;
+	int size = sizeof(*(ptr));
+
+	smp_mb();
+	switch (size) {
+	case 1:
+		oldval = __cmpxchg8(ptr, old, new);
+		break;
+	case 2:
+		oldval = __cmpxchg16(ptr, old, new);
+		break;
+	default:
+		oldval = __cmpxchg(ptr, old, new, size);
+		break;
+	}
+	smp_mb();
+
+	return oldval;
+}
 
 #endif
diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
index 8b1f37b..2032ee6 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -16,7 +16,37 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
 	return raw_irqs_disabled_flags(regs->ARM_cpsr);
 }
 
-#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr),	\
+#ifdef CONFIG_GENERIC_ATOMIC64
+/* if CONFIG_GENERIC_ATOMIC64 is defined we cannot use the generic
+ * atomic64_xchg function because it is implemented using spin locks.
+ * Here we need proper atomic instructions to read and write memory
+ * shared with the hypervisor.
+ */
+static inline u64 xen_atomic64_xchg(atomic64_t *ptr, u64 new)
+{
+	u64 result;
+	unsigned long tmp;
+
+	smp_mb();
+
+	__asm__ __volatile__("@ xen_atomic64_xchg\n"
+"1:	ldrexd	%0, %H0, [%3]\n"
+"	strexd	%1, %4, %H4, [%3]\n"
+"	teq	%1, #0\n"
+"	bne	1b"
+	: "=&r" (result), "=&r" (tmp), "+Qo" (ptr->counter)
+	: "r" (&ptr->counter), "r" (new)
+	: "cc");
+
+	smp_mb();
+
+	return result;
+}
+#else
+#define xen_atomic64_xchg atomic64_xchg
+#endif
+
+#define xchg_xen_ulong(ptr, val) xen_atomic64_xchg(container_of((ptr),	\
 							    atomic64_t,	\
 							    counter), (val))
 

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-20 15:32             ` Stefano Stabellini
@ 2014-01-21 11:08               ` Will Deacon
  2014-01-21 12:08                 ` Stefano Stabellini
  0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2014-01-21 11:08 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Arnd Bergmann, linux-arm-kernel, linux, Catalin Marinas,
	gang.chen, linux-kernel, jaccon.bastiaansen

On Mon, Jan 20, 2014 at 03:32:48PM +0000, Stefano Stabellini wrote:
> On Thu, 16 Jan 2014, Will Deacon wrote:
> > For the xchg part, yes, that looks a lot better. I don't like the #undef
> > CONFIG_CPU_V6 though, can that be rewritten to use __LINUX_ARM_ARCH__?
> 
> The problem is that the 1 and 2 byte parameter size cases in __cmpxchg
> are ifdef'ed CONFIG_CPU_V6 but drivers/xen/grant-table.c needs them.
> 
> So we can either undef CONFIG_CPU_V6 in grant-table.c or call a
> different function.
> 
> If I switch from ifdef CONFIG_CPU_V6 to if __LINUX_ARM_ARCH__ > 6 in
> __cmpxchg, we still have the problem that if __LINUX_ARM_ARCH__ == 6,
> grant-table.c doesn't compile.
> 
> Maybe the approach taken by the other patch for cmpxchg is better, see
> below.

Yes, I prefer this approach. Minor comment below.

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c1f1a7e..ae54ae0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1881,8 +1881,7 @@ config XEN_DOM0
>  config XEN
>  	bool "Xen guest support on ARM (EXPERIMENTAL)"
>  	depends on ARM && AEABI && OF
> -	depends on CPU_V7 && !CPU_V6
> -	depends on !GENERIC_ATOMIC64
> +	depends on CPU_V7
>  	select ARM_PSCI
>  	select SWIOTLB_XEN
>  	help
> diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h
> index df2fbba..cc8a4a2 100644
> --- a/arch/arm/include/asm/cmpxchg.h
> +++ b/arch/arm/include/asm/cmpxchg.h
> @@ -133,6 +133,44 @@ extern void __bad_cmpxchg(volatile void *ptr, int size);
>   * cmpxchg only support 32-bits operands on ARMv6.
>   */
>  
> +static inline unsigned long __cmpxchg8(volatile void *ptr, unsigned long old,
> +				      unsigned long new)
> +{
> +	unsigned long oldval, res;
> +
> +	do {
> +		asm volatile("@ __cmpxchg1\n"
> +		"	ldrexb	%1, [%2]\n"
> +		"	mov	%0, #0\n"
> +		"	teq	%1, %3\n"
> +		"	strexbeq %0, %4, [%2]\n"
> +			: "=&r" (res), "=&r" (oldval)
> +			: "r" (ptr), "Ir" (old), "r" (new)
> +			: "memory", "cc");
> +	} while (res);
> +
> +	return oldval;
> +}
> +
> +static inline unsigned long __cmpxchg16(volatile void *ptr, unsigned long old,
> +				      unsigned long new)
> +{
> +	unsigned long oldval, res;
> +
> +	do {
> +		asm volatile("@ __cmpxchg1\n"

Can you fix this comment while you're here please?

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN
  2014-01-21 11:08               ` Will Deacon
@ 2014-01-21 12:08                 ` Stefano Stabellini
  0 siblings, 0 replies; 12+ messages in thread
From: Stefano Stabellini @ 2014-01-21 12:08 UTC (permalink / raw)
  To: Will Deacon
  Cc: Stefano Stabellini, Arnd Bergmann, linux-arm-kernel, linux,
	Catalin Marinas, gang.chen, linux-kernel, jaccon.bastiaansen

On Tue, 21 Jan 2014, Will Deacon wrote:
> On Mon, Jan 20, 2014 at 03:32:48PM +0000, Stefano Stabellini wrote:
> > On Thu, 16 Jan 2014, Will Deacon wrote:
> > > For the xchg part, yes, that looks a lot better. I don't like the #undef
> > > CONFIG_CPU_V6 though, can that be rewritten to use __LINUX_ARM_ARCH__?
> > 
> > The problem is that the 1 and 2 byte parameter size cases in __cmpxchg
> > are ifdef'ed CONFIG_CPU_V6 but drivers/xen/grant-table.c needs them.
> > 
> > So we can either undef CONFIG_CPU_V6 in grant-table.c or call a
> > different function.
> > 
> > If I switch from ifdef CONFIG_CPU_V6 to if __LINUX_ARM_ARCH__ > 6 in
> > __cmpxchg, we still have the problem that if __LINUX_ARM_ARCH__ == 6,
> > grant-table.c doesn't compile.
> > 
> > Maybe the approach taken by the other patch for cmpxchg is better, see
> > below.
> 
> Yes, I prefer this approach. Minor comment below.
> 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index c1f1a7e..ae54ae0 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1881,8 +1881,7 @@ config XEN_DOM0
> >  config XEN
> >  	bool "Xen guest support on ARM (EXPERIMENTAL)"
> >  	depends on ARM && AEABI && OF
> > -	depends on CPU_V7 && !CPU_V6
> > -	depends on !GENERIC_ATOMIC64
> > +	depends on CPU_V7
> >  	select ARM_PSCI
> >  	select SWIOTLB_XEN
> >  	help
> > diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h
> > index df2fbba..cc8a4a2 100644
> > --- a/arch/arm/include/asm/cmpxchg.h
> > +++ b/arch/arm/include/asm/cmpxchg.h
> > @@ -133,6 +133,44 @@ extern void __bad_cmpxchg(volatile void *ptr, int size);
> >   * cmpxchg only support 32-bits operands on ARMv6.
> >   */
> >  
> > +static inline unsigned long __cmpxchg8(volatile void *ptr, unsigned long old,
> > +				      unsigned long new)
> > +{
> > +	unsigned long oldval, res;
> > +
> > +	do {
> > +		asm volatile("@ __cmpxchg1\n"
> > +		"	ldrexb	%1, [%2]\n"
> > +		"	mov	%0, #0\n"
> > +		"	teq	%1, %3\n"
> > +		"	strexbeq %0, %4, [%2]\n"
> > +			: "=&r" (res), "=&r" (oldval)
> > +			: "r" (ptr), "Ir" (old), "r" (new)
> > +			: "memory", "cc");
> > +	} while (res);
> > +
> > +	return oldval;
> > +}
> > +
> > +static inline unsigned long __cmpxchg16(volatile void *ptr, unsigned long old,
> > +				      unsigned long new)
> > +{
> > +	unsigned long oldval, res;
> > +
> > +	do {
> > +		asm volatile("@ __cmpxchg1\n"
> 
> Can you fix this comment while you're here please?

OK, I'll use @ __cmpxchg16 and @ __cmpxchg8.
I'll resend as a separate patch.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-01-21 12:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-08 18:00 [PATCH v3] arm: remove !CPU_V6 and !GENERIC_ATOMIC64 build dependencies for XEN Stefano Stabellini
2014-01-09 10:30 ` Will Deacon
2014-01-09 11:04   ` Arnd Bergmann
2014-01-09 12:47     ` Stefano Stabellini
2014-01-09 18:42       ` Will Deacon
2014-01-10 16:48         ` Chen Gang F T
2014-01-13  8:12           ` Jaccon Bastiaansen
2014-01-16 16:27         ` Stefano Stabellini
2014-01-16 19:31           ` Will Deacon
2014-01-20 15:32             ` Stefano Stabellini
2014-01-21 11:08               ` Will Deacon
2014-01-21 12:08                 ` Stefano Stabellini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).