All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
@ 2013-04-08 12:34 Vladimir Danushevsky
  2013-04-08 13:21 ` Will Deacon
  2013-04-08 14:24 ` Russell King - ARM Linux
  0 siblings, 2 replies; 13+ messages in thread
From: Vladimir Danushevsky @ 2013-04-08 12:34 UTC (permalink / raw)
  To: linux-arm-kernel

From: Vladimir Danushevsky <vladidan@oracle.com>

Hi All,

One of the indirect LPAE features in a single-copy atomicity of LDRD/STRD instructions. This information is useful for some dynamic code generating applications (e.g. JIT compilers) to produce a more efficient access mechanism to atomic/volatile operands. 

The feature is part of the design scheme therefore it's not related to the platform configuration i.e. whether CONFIG_ARM_LPAE is enabled or not.

The patch exposes that information through hwcap entry of an Auxiliary Vector.



Signed-off-by: Vladimir Danushevsky <vladidan@oracle.com>

---

--- linux-3.8.4_vanilla/arch/arm/include/uapi/asm/hwcap.h       2013-03-20 16:11:19.000000000 -0400

+++ linux-3.8.4/arch/arm/include/uapi/asm/hwcap.h       2013-03-26 16:32:18.266043699 -0400

@@ -25,6 +25,7 @@

 #define HWCAP_IDIVT    (1 << 18)

 #define HWCAP_VFPD32   (1 << 19)       /* set if VFP has 32 regs (not 16) */

 #define HWCAP_IDIV     (HWCAP_IDIVA | HWCAP_IDIVT)

+#define HWCAP_ATOMICD   (1 << 20)       /* ldrd/strd single-copy atomicity */



 #endif /* _UAPI__ASMARM_HWCAP_H */


--- linux-3.8.4_vanilla/arch/arm/kernel/setup.c 2013-03-20 16:11:19.000000000 -0400

+++ linux-3.8.4/arch/arm/kernel/setup.c 2013-04-03 15:42:31.580525201 -0400

@@ -487,6 +487,10 @@ static void __init setup_processor(void)

        elf_hwcap &= ~HWCAP_THUMB;

 #endif


+        /* check LPAE presence */ 

+        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)

+          elf_hwcap |= HWCAP_ATOMICD;

+

        feat_v6_fixup();


        cacheid_init();

---

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 12:34 [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV Vladimir Danushevsky
@ 2013-04-08 13:21 ` Will Deacon
  2013-04-08 15:11   ` Vladimir Danushevsky
  2013-04-08 14:24 ` Russell King - ARM Linux
  1 sibling, 1 reply; 13+ messages in thread
From: Will Deacon @ 2013-04-08 13:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Apr 08, 2013 at 01:34:28PM +0100, Vladimir Danushevsky wrote:
> From: Vladimir Danushevsky <vladidan@oracle.com>
> 
> Hi All,

Hello,

> One of the indirect LPAE features in a single-copy atomicity of LDRD/STRD instructions. This information is useful for some dynamic code generating applications (e.g. JIT compilers) to produce a more efficient access mechanism to atomic/volatile operands. 
> 
> The feature is part of the design scheme therefore it's not related to the platform configuration i.e. whether CONFIG_ARM_LPAE is enabled or not.
> 
> The patch exposes that information through hwcap entry of an Auxiliary Vector.

Hmm, your commit text and patch format are both completely mangled.
Furthermore, you've taken your patch against a -stable kernel, rather than
the latest mainline release, so it doesn't apply anymore anyway.

However, I see what you're trying to do, and I've recently made use of this
in our atomic64 implementation so that we avoid the exclusive instructions
for atomic64_{read,set}.

> --- linux-3.8.4_vanilla/arch/arm/kernel/setup.c 2013-03-20 16:11:19.000000000 -0400
> 
> +++ linux-3.8.4/arch/arm/kernel/setup.c 2013-04-03 15:42:31.580525201 -0400
> 
> @@ -487,6 +487,10 @@ static void __init setup_processor(void)
> 
>         elf_hwcap &= ~HWCAP_THUMB;
> 
>  #endif
> 
> 
> +        /* check LPAE presence */ 
> 
> +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
> 
> +          elf_hwcap |= HWCAP_ATOMICD;

This is broken -- you're checking for supersection support (why?) rather
than checking the vmsa field in MMFR0.

How about something like the patch below instead?

Will

--->8

diff --git a/arch/arm/include/uapi/asm/hwcap.h b/arch/arm/include/uapi/asm/hwcap.h
index 3688fd1..ecf0c6f 100644
--- a/arch/arm/include/uapi/asm/hwcap.h
+++ b/arch/arm/include/uapi/asm/hwcap.h
@@ -25,6 +25,7 @@
 #define HWCAP_IDIVT    (1 << 18)
 #define HWCAP_VFPD32   (1 << 19)       /* set if VFP has 32 regs (not 16) */
 #define HWCAP_IDIV     (HWCAP_IDIVA | HWCAP_IDIVT)
+#define HWCAP_ATOMICD  (1 << 20)
 
 
 #endif /* _UAPI__ASMARM_HWCAP_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index d343a6c..cae7b9e 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -355,7 +355,7 @@ void __init early_print(const char *str, ...)
 
 static void __init cpuid_init_hwcaps(void)
 {
-       unsigned int divide_instrs;
+       unsigned int divide_instrs, vmsa;
 
        if (cpu_architecture() < CPU_ARCH_ARMv7)
                return;
@@ -368,6 +368,11 @@ static void __init cpuid_init_hwcaps(void)
        case 1:
                elf_hwcap |= HWCAP_IDIVT;
        }
+
+       /* LPAE implies atomic ldrd/strd instructions */
+       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
+       if (vmsa >= 5)
+               elf_hwcap |= HWCAP_ATOMICD;
 }
 
 static void __init feat_v6_fixup(void)
@@ -864,6 +869,7 @@ static const char *hwcap_str[] = {
        "vfpv4",
        "idiva",
        "idivt",
+       "atomicd",
        NULL
 };
 

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 12:34 [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV Vladimir Danushevsky
  2013-04-08 13:21 ` Will Deacon
@ 2013-04-08 14:24 ` Russell King - ARM Linux
  2013-04-08 15:31   ` Vladimir Danushevsky
  2013-04-08 15:57   ` Will Deacon
  1 sibling, 2 replies; 13+ messages in thread
From: Russell King - ARM Linux @ 2013-04-08 14:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Apr 08, 2013 at 08:34:28AM -0400, Vladimir Danushevsky wrote:
> From: Vladimir Danushevsky <vladidan@oracle.com>

Please don't obfuscate your email address like that - git won't parse it.

> Hi All,

We don't include such salutations in the commit message.

> One of the indirect LPAE features in a single-copy atomicity of LDRD/STRD
> instructions. This information is useful for some dynamic code generating
> applications (e.g. JIT compilers) to produce a more efficient access
> mechanism to atomic/volatile operands.
> 
> The feature is part of the design scheme therefore it's not related to the
> platform configuration i.e. whether CONFIG_ARM_LPAE is enabled or not.
> 
> The patch exposes that information through hwcap entry of an Auxiliary
> Vector.

I think we're heading towards running out of hwcap bits to represent all
these different features.  Not there yet, but after this we only have 11
spare bits.

Maybe we need to switch to a more x86-centric method where we report this
stuff in /proc/cpuinfo, and userspace parses this information out.  If
glibc already has code in there for x86, maybe we can reuse that?

> Signed-off-by: Vladimir Danushevsky <vladidan@oracle.com>

Again, don't obfuscate.

> ---

diffstat?

> 
> --- linux-3.8.4_vanilla/arch/arm/include/uapi/asm/hwcap.h       2013-03-20 16:11:19.000000000 -0400
> 
> +++ linux-3.8.4/arch/arm/include/uapi/asm/hwcap.h       2013-03-26 16:32:18.266043699 -0400

This double-line spacing makes this patch impossible to apply.

> +        /* check LPAE presence */ 

If it's worth a comment, it's worth making it a good comment.
> 
> +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
> 
> +          elf_hwcap |= HWCAP_ATOMICD;

We use tabs for indentation, not two spaces.  Also, on the face of it
ATOMICD isn't obvious what it means.  Maybe HWCAP_LDRDSTRD would be
better - it's only one character longer and truely reflects what you're
trying to report - the presence of these two instructions.

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 13:21 ` Will Deacon
@ 2013-04-08 15:11   ` Vladimir Danushevsky
  0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Danushevsky @ 2013-04-08 15:11 UTC (permalink / raw)
  To: linux-arm-kernel


On Apr 8, 2013, at 9:21 AM, Will Deacon wrote:

> On Mon, Apr 08, 2013 at 01:34:28PM +0100, Vladimir Danushevsky wrote:
>> From: Vladimir Danushevsky <vladidan@oracle.com>
>> 
>> Hi All,
> 
> Hello,
> 
>> One of the indirect LPAE features in a single-copy atomicity of LDRD/STRD instructions. This information is useful for some dynamic code generating applications (e.g. JIT compilers) to produce a more efficient access mechanism to atomic/volatile operands. 
>> 
>> The feature is part of the design scheme therefore it's not related to the platform configuration i.e. whether CONFIG_ARM_LPAE is enabled or not.
>> 
>> The patch exposes that information through hwcap entry of an Auxiliary Vector.
> 
> Hmm, your commit text and patch format are both completely mangled.
> Furthermore, you've taken your patch against a -stable kernel, rather than
> the latest mainline release, so it doesn't apply anymore anyway.

Apologize for mutilating the patch text/body. My first post.

> 
> However, I see what you're trying to do, and I've recently made use of this
> in our atomic64 implementation so that we avoid the exclusive instructions
> for atomic64_{read,set}.
> 
>> --- linux-3.8.4_vanilla/arch/arm/kernel/setup.c 2013-03-20 16:11:19.000000000 -0400
>> 
>> +++ linux-3.8.4/arch/arm/kernel/setup.c 2013-04-03 15:42:31.580525201 -0400
>> 
>> @@ -487,6 +487,10 @@ static void __init setup_processor(void)
>> 
>>        elf_hwcap &= ~HWCAP_THUMB;
>> 
>> #endif
>> 
>> 
>> +        /* check LPAE presence */ 
>> 
>> +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
>> 
>> +          elf_hwcap |= HWCAP_ATOMICD;
> 
> This is broken -- you're checking for supersection support (why?) rather
> than checking the vmsa field in MMFR0.
> 
> How about something like the patch below instead?

The below patch is indeed more correct than the one originally suggested as long as MMFR0.VMSA values continue to represent an accumulating list of features (i.e. new supported values also imply long page format) which is probably true.

Thanks,
Vlad 

> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm/include/uapi/asm/hwcap.h b/arch/arm/include/uapi/asm/hwcap.h
> index 3688fd1..ecf0c6f 100644
> --- a/arch/arm/include/uapi/asm/hwcap.h
> +++ b/arch/arm/include/uapi/asm/hwcap.h
> @@ -25,6 +25,7 @@
> #define HWCAP_IDIVT    (1 << 18)
> #define HWCAP_VFPD32   (1 << 19)       /* set if VFP has 32 regs (not 16) */
> #define HWCAP_IDIV     (HWCAP_IDIVA | HWCAP_IDIVT)
> +#define HWCAP_ATOMICD  (1 << 20)
> 
> 
> #endif /* _UAPI__ASMARM_HWCAP_H */
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index d343a6c..cae7b9e 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -355,7 +355,7 @@ void __init early_print(const char *str, ...)
> 
> static void __init cpuid_init_hwcaps(void)
> {
> -       unsigned int divide_instrs;
> +       unsigned int divide_instrs, vmsa;
> 
>        if (cpu_architecture() < CPU_ARCH_ARMv7)
>                return;
> @@ -368,6 +368,11 @@ static void __init cpuid_init_hwcaps(void)
>        case 1:
>                elf_hwcap |= HWCAP_IDIVT;
>        }
> +
> +       /* LPAE implies atomic ldrd/strd instructions */
> +       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
> +       if (vmsa >= 5)
> +               elf_hwcap |= HWCAP_ATOMICD;
> }
> 
> static void __init feat_v6_fixup(void)
> @@ -864,6 +869,7 @@ static const char *hwcap_str[] = {
>        "vfpv4",
>        "idiva",
>        "idivt",
> +       "atomicd",
>        NULL
> };
> 

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 14:24 ` Russell King - ARM Linux
@ 2013-04-08 15:31   ` Vladimir Danushevsky
  2013-04-08 15:57   ` Will Deacon
  1 sibling, 0 replies; 13+ messages in thread
From: Vladimir Danushevsky @ 2013-04-08 15:31 UTC (permalink / raw)
  To: linux-arm-kernel


On Apr 8, 2013, at 10:24 AM, Russell King - ARM Linux wrote:

> On Mon, Apr 08, 2013 at 08:34:28AM -0400, Vladimir Danushevsky wrote:
>> From: Vladimir Danushevsky <vladidan@oracle.com>
> 
> Please don't obfuscate your email address like that - git won't parse it.
> 
>> Hi All,
> 
> We don't include such salutations in the commit message.
> 
I stand corrected for the comments above. Thanks.

>> One of the indirect LPAE features in a single-copy atomicity of LDRD/STRD
>> instructions. This information is useful for some dynamic code generating
>> applications (e.g. JIT compilers) to produce a more efficient access
>> mechanism to atomic/volatile operands.
>> 
>> The feature is part of the design scheme therefore it's not related to the
>> platform configuration i.e. whether CONFIG_ARM_LPAE is enabled or not.
>> 
>> The patch exposes that information through hwcap entry of an Auxiliary
>> Vector.
> 
> I think we're heading towards running out of hwcap bits to represent all
> these different features.  Not there yet, but after this we only have 11
> spare bits.

yes we will eventually run out of the bits, but until that time only user level features need to be exposed through AUXV i.e. LPAE or HYP should not interest an user application. 

> 
> Maybe we need to switch to a more x86-centric method where we report this
> stuff in /proc/cpuinfo, and userspace parses this information out.  If
> glibc already has code in there for x86, maybe we can reuse that?

Doesn't x86 still report first 32 bits of HWCAP in the AUXV?

> 
>> Signed-off-by: Vladimir Danushevsky <vladidan@oracle.com>
> 
> Again, don't obfuscate.

Noted. 

Thanks,
Vlad
> 
>> ---
> 
> diffstat?
> 
>> 
>> --- linux-3.8.4_vanilla/arch/arm/include/uapi/asm/hwcap.h       2013-03-20 16:11:19.000000000 -0400
>> 
>> +++ linux-3.8.4/arch/arm/include/uapi/asm/hwcap.h       2013-03-26 16:32:18.266043699 -0400
> 
> This double-line spacing makes this patch impossible to apply.
> 
>> +        /* check LPAE presence */ 
> 
> If it's worth a comment, it's worth making it a good comment.
>> 
>> +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
>> 
>> +          elf_hwcap |= HWCAP_ATOMICD;
> 
> We use tabs for indentation, not two spaces.  Also, on the face of it
> ATOMICD isn't obvious what it means.  Maybe HWCAP_LDRDSTRD would be
> better - it's only one character longer and truely reflects what you're
> trying to report - the presence of these two instructions.

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 14:24 ` Russell King - ARM Linux
  2013-04-08 15:31   ` Vladimir Danushevsky
@ 2013-04-08 15:57   ` Will Deacon
  2013-04-12 20:58     ` Vladimir Danushevsky
  1 sibling, 1 reply; 13+ messages in thread
From: Will Deacon @ 2013-04-08 15:57 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Russell,

On Mon, Apr 08, 2013 at 03:24:06PM +0100, Russell King - ARM Linux wrote:
> I think we're heading towards running out of hwcap bits to represent all
> these different features.  Not there yet, but after this we only have 11
> spare bits.
> 
> Maybe we need to switch to a more x86-centric method where we report this
> stuff in /proc/cpuinfo, and userspace parses this information out.  If
> glibc already has code in there for x86, maybe we can reuse that?

Why can't we just pass multiple AT_HWCAP entries in the auxv? It would
require a small change to the ELF loader to allow an architecture to provide
more than just ELF_HWCAP, but it's fairly straightforward and easy to
propogate without breaking backwards compatibility.

> > +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
> > 
> > +          elf_hwcap |= HWCAP_ATOMICD;
> 
> We use tabs for indentation, not two spaces.  Also, on the face of it
> ATOMICD isn't obvious what it means.  Maybe HWCAP_LDRDSTRD would be
> better - it's only one character longer and truely reflects what you're
> trying to report - the presence of these two instructions.

Actually, it's not just the presence of those instructions -- it's their
behaviour wrt atomicity. They are only guaranteed to be atomic if the CPU in
question supports LPAE. We could call it "lpae" but it might be set even
when the kernel is using the short-descriptor format.

Will

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-08 15:57   ` Will Deacon
@ 2013-04-12 20:58     ` Vladimir Danushevsky
  2013-04-16 16:04       ` Catalin Marinas
  2013-04-16 17:22       ` Will Deacon
  0 siblings, 2 replies; 13+ messages in thread
From: Vladimir Danushevsky @ 2013-04-12 20:58 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Will,

On Apr 8, 2013, at 11:57 AM, Will Deacon wrote:

> Hi Russell,
> 
> On Mon, Apr 08, 2013 at 03:24:06PM +0100, Russell King - ARM Linux wrote:
>> I think we're heading towards running out of hwcap bits to represent all
>> these different features.  Not there yet, but after this we only have 11
>> spare bits.
>> 
>> Maybe we need to switch to a more x86-centric method where we report this
>> stuff in /proc/cpuinfo, and userspace parses this information out.  If
>> glibc already has code in there for x86, maybe we can reuse that?
> 
> Why can't we just pass multiple AT_HWCAP entries in the auxv? It would
> require a small change to the ELF loader to allow an architecture to provide
> more than just ELF_HWCAP, but it's fairly straightforward and easy to
> propogate without breaking backwards compatibility.
> 
>>> +        if (((read_cpuid_ext(CPUID_EXT_MMFR3) >> 28) & 0xf) > 0)
>>> 
>>> +          elf_hwcap |= HWCAP_ATOMICD;
>> 
>> We use tabs for indentation, not two spaces.  Also, on the face of it
>> ATOMICD isn't obvious what it means.  Maybe HWCAP_LDRDSTRD would be
>> better - it's only one character longer and truely reflects what you're
>> trying to report - the presence of these two instructions.
> 
> Actually, it's not just the presence of those instructions -- it's their
> behaviour wrt atomicity. They are only guaranteed to be atomic if the CPU in
> question supports LPAE. We could call it "lpae" but it might be set even
> when the kernel is using the short-descriptor format.

I also think that ATOMICD would describe the intended behavior better.

Below is a modified patch based on suggestions so far:


Signed-off-by: Vladimir Danushevsky <vladimir.danushevsky@oracle.com>

 include/uapi/asm/hwcap.h |    1 +
 kernel/setup.c           |   13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)


diff -uprN -X linux-3.9-rc6/Documentation/dontdiff linux-3.9-rc6-vanilla/arch/arm/include/uapi/asm/hwcap.h linux-3.9-rc6/arch/arm/include/uapi/asm/hwcap.h
--- linux-3.9-rc6-vanilla/arch/arm/include/uapi/asm/hwcap.h	2013-04-07 23:49:54.000000000 -0400
+++ linux-3.9-rc6/arch/arm/include/uapi/asm/hwcap.h	2013-04-08 12:01:24.952168593 -0400
@@ -25,6 +25,7 @@
 #define HWCAP_IDIVT	(1 << 18)
 #define HWCAP_VFPD32	(1 << 19)	/* set if VFP has 32 regs (not 16) */
 #define HWCAP_IDIV	(HWCAP_IDIVA | HWCAP_IDIVT)
+#define HWCAP_ATOMICD	(1 << 20)	/* atomic behavior of ldrd/strd */
 
 
 #endif /* _UAPI__ASMARM_HWCAP_H */
diff -uprN -X linux-3.9-rc6/Documentation/dontdiff linux-3.9-rc6-vanilla/arch/arm/kernel/setup.c linux-3.9-rc6/arch/arm/kernel/setup.c
--- linux-3.9-rc6-vanilla/arch/arm/kernel/setup.c	2013-04-07 23:49:54.000000000 -0400
+++ linux-3.9-rc6/arch/arm/kernel/setup.c	2013-04-08 12:42:26.145583364 -0400
@@ -355,7 +355,7 @@ void __init early_print(const char *str,
 
 static void __init cpuid_init_hwcaps(void)
 {
-	unsigned int divide_instrs;
+	unsigned int divide_instrs, vmsa;
 
 	if (cpu_architecture() < CPU_ARCH_ARMv7)
 		return;
@@ -368,6 +368,14 @@ static void __init cpuid_init_hwcaps(voi
 	case 1:
 		elf_hwcap |= HWCAP_IDIVT;
 	}
+
+        /*
+         * LDRD/STRD instructions are single-copy atomic on LPAE enabled targets
+         * as long as double-word alignment is kept. Report that feature.
+         */
+        vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
+        if (vmsa >= 5)
+                elf_hwcap |= HWCAP_ATOMICD;
 }
 
 static void __init feat_v6_fixup(void)
@@ -506,7 +514,7 @@ static void __init setup_processor(void)
 #ifndef CONFIG_ARM_THUMB
 	elf_hwcap &= ~(HWCAP_THUMB | HWCAP_IDIVT);
 #endif

 	feat_v6_fixup();
 
 	cacheid_init();
@@ -864,6 +872,7 @@ static const char *hwcap_str[] = {
 	"vfpv4",
 	"idiva",
 	"idivt",
+	"atomicd",
 	NULL
 };
 



> 
> Will

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-12 20:58     ` Vladimir Danushevsky
@ 2013-04-16 16:04       ` Catalin Marinas
  2013-04-16 17:22       ` Will Deacon
  1 sibling, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2013-04-16 16:04 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 12, 2013 at 09:58:43PM +0100, Vladimir Danushevsky wrote:
> Below is a modified patch based on suggestions so far:

You need to add a description, maybe just the comment that you already
added in the code.

> Signed-off-by: Vladimir Danushevsky <vladimir.danushevsky@oracle.com>
> 
>  include/uapi/asm/hwcap.h |    1 +
>  kernel/setup.c           |   13 +++++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)

Otherwise,

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-12 20:58     ` Vladimir Danushevsky
  2013-04-16 16:04       ` Catalin Marinas
@ 2013-04-16 17:22       ` Will Deacon
  2013-04-17  9:48         ` Will Deacon
  2013-04-18 16:22         ` Catalin Marinas
  1 sibling, 2 replies; 13+ messages in thread
From: Will Deacon @ 2013-04-16 17:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 12, 2013 at 09:58:43PM +0100, Vladimir Danushevsky wrote:
> On Apr 8, 2013, at 11:57 AM, Will Deacon wrote:
> > Actually, it's not just the presence of those instructions -- it's their
> > behaviour wrt atomicity. They are only guaranteed to be atomic if the CPU in
> > question supports LPAE. We could call it "lpae" but it might be set even
> > when the kernel is using the short-descriptor format.
> 
> I also think that ATOMICD would describe the intended behavior better.

I started having second thoughts about the name, on the offchance that some
other feature introduced with LPAE is found to be useful to userspace. Then
userspace would end up checking ATOMICD in order to determine something
different, which is counter-intuitive.

Maybe there is no other `killer feature' for userspace with LPAE, but at
least we'd be reporting what the hardware says, rather than the small bit
which we're interested in.

Patch below...

Will

--->8

commit c6eaaa758c7956b18aa0cfabe9500ef73514b319
Author: Will Deacon <will.deacon@arm.com>
Date:   Mon Apr 8 17:13:12 2013 +0100

    ARM: elf: add new hwcap for identifying atomic ldrd/strd instructions
    
    CPUs implementing LPAE have atomic ldrd/strd instructions, meaning that
    userspace software can avoid having to use the exclusive variants of
    these instructions if they wish.
    
    This patch advertises the atomicity of these instructions via the
    hwcaps, so userspace can detect this CPU feature.
    
    Signed-off-by: Will Deacon <will.deacon@arm.com>

diff --git a/arch/arm/include/uapi/asm/hwcap.h b/arch/arm/include/uapi/asm/hwcap.h
index 3688fd1..6d34d08 100644
--- a/arch/arm/include/uapi/asm/hwcap.h
+++ b/arch/arm/include/uapi/asm/hwcap.h
@@ -25,6 +25,6 @@
 #define HWCAP_IDIVT    (1 << 18)
 #define HWCAP_VFPD32   (1 << 19)       /* set if VFP has 32 regs (not 16) */
 #define HWCAP_IDIV     (HWCAP_IDIVA | HWCAP_IDIVT)
-
+#define HWCAP_LPAE     (1 << 20)
 
 #endif /* _UAPI__ASMARM_HWCAP_H */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index eaa7900..57a53c5 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -356,7 +356,7 @@ void __init early_print(const char *str, ...)
 
 static void __init cpuid_init_hwcaps(void)
 {
-       unsigned int divide_instrs;
+       unsigned int divide_instrs, vmsa;
 
        if (cpu_architecture() < CPU_ARCH_ARMv7)
                return;
@@ -369,6 +369,11 @@ static void __init cpuid_init_hwcaps(void)
        case 1:
                elf_hwcap |= HWCAP_IDIVT;
        }
+
+       /* LPAE implies atomic ldrd/strd instructions */
+       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
+       if (vmsa >= 5)
+               elf_hwcap |= HWCAP_LPAE;
 }
 
 static void __init feat_v6_fixup(void)
@@ -875,6 +880,7 @@ static const char *hwcap_str[] = {
        "vfpv4",
        "idiva",
        "idivt",
+       "atomicd",
        NULL
 };
 

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-16 17:22       ` Will Deacon
@ 2013-04-17  9:48         ` Will Deacon
  2013-04-18 16:22         ` Catalin Marinas
  1 sibling, 0 replies; 13+ messages in thread
From: Will Deacon @ 2013-04-17  9:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 16, 2013 at 06:22:58PM +0100, Will Deacon wrote:
> Patch below...

[...]

> +#define HWCAP_LPAE     (1 << 20)

[...]

> @@ -875,6 +880,7 @@ static const char *hwcap_str[] = {
>         "vfpv4",
>         "idiva",
>         "idivt",
> +       "atomicd",
>         NULL
>  };

You can tell I was in two minds about this, but that string should read
"lpae".

Will

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-16 17:22       ` Will Deacon
  2013-04-17  9:48         ` Will Deacon
@ 2013-04-18 16:22         ` Catalin Marinas
  2013-04-22 14:17           ` Russell King - ARM Linux
  1 sibling, 1 reply; 13+ messages in thread
From: Catalin Marinas @ 2013-04-18 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 16, 2013 at 06:22:59PM +0100, Will Deacon wrote:
> On Fri, Apr 12, 2013 at 09:58:43PM +0100, Vladimir Danushevsky wrote:
> > On Apr 8, 2013, at 11:57 AM, Will Deacon wrote:
> > > Actually, it's not just the presence of those instructions -- it's their
> > > behaviour wrt atomicity. They are only guaranteed to be atomic if the CPU in
> > > question supports LPAE. We could call it "lpae" but it might be set even
> > > when the kernel is using the short-descriptor format.
> > 
> > I also think that ATOMICD would describe the intended behavior better.
> 
> I started having second thoughts about the name, on the offchance that some
> other feature introduced with LPAE is found to be useful to userspace. Then
> userspace would end up checking ATOMICD in order to determine something
> different, which is counter-intuitive.
> 
> Maybe there is no other `killer feature' for userspace with LPAE, but at
> least we'd be reporting what the hardware says, rather than the small bit
> which we're interested in.
> 
> Patch below...
> 
> Will
> 
> --->8
> 
> commit c6eaaa758c7956b18aa0cfabe9500ef73514b319
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Mon Apr 8 17:13:12 2013 +0100
> 
>     ARM: elf: add new hwcap for identifying atomic ldrd/strd instructions
>     
>     CPUs implementing LPAE have atomic ldrd/strd instructions, meaning that
>     userspace software can avoid having to use the exclusive variants of
>     these instructions if they wish.
>     
>     This patch advertises the atomicity of these instructions via the
>     hwcaps, so userspace can detect this CPU feature.
>     
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> diff --git a/arch/arm/include/uapi/asm/hwcap.h b/arch/arm/include/uapi/asm/hwcap.h
> index 3688fd1..6d34d08 100644
> --- a/arch/arm/include/uapi/asm/hwcap.h
> +++ b/arch/arm/include/uapi/asm/hwcap.h
> @@ -25,6 +25,6 @@
>  #define HWCAP_IDIVT    (1 << 18)
>  #define HWCAP_VFPD32   (1 << 19)       /* set if VFP has 32 regs (not 16) */
>  #define HWCAP_IDIV     (HWCAP_IDIVA | HWCAP_IDIVT)
> -
> +#define HWCAP_LPAE     (1 << 20)

I would rather add individual entries like atomicd. User space does not
need to know whether LPAE is supported or not.

-- 
Catalin

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-18 16:22         ` Catalin Marinas
@ 2013-04-22 14:17           ` Russell King - ARM Linux
  2013-04-22 14:28             ` Catalin Marinas
  0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2013-04-22 14:17 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Apr 18, 2013 at 05:22:33PM +0100, Catalin Marinas wrote:
> I would rather add individual entries like atomicd. User space does not
> need to know whether LPAE is supported or not.

I'm not sure that userspace should know this detail.

| LDM, LDC, LDC2, LDRD, STM, STC, STC2, STRD, PUSH, POP, RFE, SRS, VLDM,
| VLDR, VSTM, and VSTR instructions are executed as a sequence of word-
| aligned word accesses. Each 32-bit word access is guaranteed to be
| single-copy atomic. The architecture does not require subsequences
| of two or more word accesses from the sequence to be single-copy
| atomic.
| 
| In an implementation that includes the Large Physical Address Extension,
| LDRD and STRD accesses to 64-bit aligned locations are 64-bit single-copy
| atomic as seen by translation table walks and accesses to translation
| tables.
| ...
| Load/store multiple instructions, such as LDM, LDRD, STM, and STRD,
| generate multiple word accesses, each of which is a separate access
| for the purpose of determining ordering.

Note carefully the wording in the ARM ARM in the second paragraph -
it's not saying that LDRD and STRD accesses are single-copy atomic
to all viewers, but it's limited to the translation tables.

Sure, you can argue that the translation tables are just normal memory
accesses, but in that case why use the above wording if not to permit
them to be multiple-copy accesses as stated in the paragraph above
and elsewhere in the ARM ARM in the paragraph below.

Maybe an error in the ARM ARM?  It doesn't seem wise through to permit
userspace to assume that LDRD/STRD are atomic in userspace given the
above wording unless the ARM ARM gets fixed.

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

* [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV
  2013-04-22 14:17           ` Russell King - ARM Linux
@ 2013-04-22 14:28             ` Catalin Marinas
  0 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2013-04-22 14:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Apr 22, 2013 at 03:17:19PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 18, 2013 at 05:22:33PM +0100, Catalin Marinas wrote:
> > I would rather add individual entries like atomicd. User space does not
> > need to know whether LPAE is supported or not.
> 
> I'm not sure that userspace should know this detail.

Which detail, LPAE or atomic double accesses?

> | LDM, LDC, LDC2, LDRD, STM, STC, STC2, STRD, PUSH, POP, RFE, SRS, VLDM,
> | VLDR, VSTM, and VSTR instructions are executed as a sequence of word-
> | aligned word accesses. Each 32-bit word access is guaranteed to be
> | single-copy atomic. The architecture does not require subsequences
> | of two or more word accesses from the sequence to be single-copy
> | atomic.
> | 
> | In an implementation that includes the Large Physical Address Extension,
> | LDRD and STRD accesses to 64-bit aligned locations are 64-bit single-copy
> | atomic as seen by translation table walks and accesses to translation
> | tables.
> | ...
> | Load/store multiple instructions, such as LDM, LDRD, STM, and STRD,
> | generate multiple word accesses, each of which is a separate access
> | for the purpose of determining ordering.
> 
> Note carefully the wording in the ARM ARM in the second paragraph -
> it's not saying that LDRD and STRD accesses are single-copy atomic
> to all viewers, but it's limited to the translation tables.

Yes, I raised this (in private I think) when Will proposed similar
kernel changes. But we discussed with the ARM architecture guys and even
though the ARM ARM states that this, in practice the CPUs just implement
full 64-bit atomic accesses.

> Sure, you can argue that the translation tables are just normal memory
> accesses, but in that case why use the above wording if not to permit
> them to be multiple-copy accesses as stated in the paragraph above
> and elsewhere in the ARM ARM in the paragraph below.
> 
> Maybe an error in the ARM ARM?  It doesn't seem wise through to permit
> userspace to assume that LDRD/STRD are atomic in userspace given the
> above wording unless the ARM ARM gets fixed.

Not an error in the ARM ARM but I don't think ARM is going to extend
this definition (not an easy process for such modifications). For
practical purposes, CPUs with LPAE implement 64-bit atomics and it's of
real benefit for some user-space cases.

Maybe a better alternative would be to add it via __v7_proc on
individual CPUs (A7 and A15).

-- 
Catalin

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

end of thread, other threads:[~2013-04-22 14:28 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-08 12:34 [PATCH] Report double word access atomicity on LPAE enabled targets through AUXV Vladimir Danushevsky
2013-04-08 13:21 ` Will Deacon
2013-04-08 15:11   ` Vladimir Danushevsky
2013-04-08 14:24 ` Russell King - ARM Linux
2013-04-08 15:31   ` Vladimir Danushevsky
2013-04-08 15:57   ` Will Deacon
2013-04-12 20:58     ` Vladimir Danushevsky
2013-04-16 16:04       ` Catalin Marinas
2013-04-16 17:22       ` Will Deacon
2013-04-17  9:48         ` Will Deacon
2013-04-18 16:22         ` Catalin Marinas
2013-04-22 14:17           ` Russell King - ARM Linux
2013-04-22 14:28             ` Catalin Marinas

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.