All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: xen-devel <xen-devel@lists.xenproject.org>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>,
	Tim Deegan <tim@xen.org>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: [PATCH 2/2] arm64: fix fls()
Date: Thu, 22 Jan 2015 13:38:54 +0000	[thread overview]
Message-ID: <54C10B7E0200007800058256@mail.emea.novell.com> (raw)
In-Reply-To: <54C108910200007800058233@mail.emea.novell.com>

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

It using CLZ on a 64-bit register while specifying the input operand as
only 32 bits wide is wrong: An operand intentionally shrunk down to 32
bits at the source level doesn't imply respective zero extension also
happens at the machine instruction level, and hence the wrong result
could get returned.

Add suitable inline assembly abstraction so that the function can
remain shared between arm32 and arm64. The need to include asm_defns.h
in bitops.h makes it necessary to adjust processor.h though - it is
generally wrong to include public headers without making sure that
integer types are properly defined. (I didn't innvestigate or try
whether the possible alternative of moving the public/arch-arm.h
inclusion down in the file would also work.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Only compile tested due to lack of suitable ARM64 test environment.

--- a/xen/include/asm-arm/asm_defns.h
+++ b/xen/include/asm-arm/asm_defns.h
@@ -7,6 +7,15 @@
 #endif
 #include <asm/processor.h>
 
+/* For generic assembly code: use macros to define operand sizes. */
+#if defined(CONFIG_ARM_32)
+# define __OP32
+#elif defined(CONFIG_ARM_64)
+# define __OP32 "w"
+#else
+# error "unknown ARM variant"
+#endif
+
 #endif /* __ARM_ASM_DEFNS_H__ */
 /*
  * Local variables:
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -9,6 +9,8 @@
 #ifndef _ARM_BITOPS_H
 #define _ARM_BITOPS_H
 
+#include <asm/asm_defns.h>
+
 /*
  * Non-atomic bit manipulation.
  *
@@ -111,9 +113,8 @@ static inline int fls(unsigned int x)
         if (__builtin_constant_p(x))
                return generic_fls(x);
 
-        asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
-        ret = BITS_PER_LONG - ret;
-        return ret;
+        asm("clz\t%"__OP32"0, %"__OP32"1" : "=r" (ret) : "r" (x));
+        return 32 - ret;
 }
 
 
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -3,6 +3,9 @@
 
 #include <asm/cpregs.h>
 #include <asm/sysregs.h>
+#ifndef __ASSEMBLY__
+#include <xen/types.h>
+#endif
 #include <public/arch-arm.h>
 
 /* MIDR Main ID Register */
@@ -220,8 +223,6 @@
 
 #ifndef __ASSEMBLY__
 
-#include <xen/types.h>
-
 struct cpuinfo_arm {
     union {
         uint32_t bits;




[-- Attachment #2: arm64-fls.patch --]
[-- Type: text/plain, Size: 2325 bytes --]

arm64: fix fls()

It using CLZ on a 64-bit register while specifying the input operand as
only 32 bits wide is wrong: An operand intentionally shrunk down to 32
bits at the source level doesn't imply respective zero extension also
happens at the machine instruction level, and hence the wrong result
could get returned.

Add suitable inline assembly abstraction so that the function can
remain shared between arm32 and arm64. The need to include asm_defns.h
in bitops.h makes it necessary to adjust processor.h though - it is
generally wrong to include public headers without making sure that
integer types are properly defined. (I didn't innvestigate or try
whether the possible alternative of moving the public/arch-arm.h
inclusion down in the file would also work.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Only compile tested due to lack of suitable ARM64 test environment.

--- a/xen/include/asm-arm/asm_defns.h
+++ b/xen/include/asm-arm/asm_defns.h
@@ -7,6 +7,15 @@
 #endif
 #include <asm/processor.h>
 
+/* For generic assembly code: use macros to define operand sizes. */
+#if defined(CONFIG_ARM_32)
+# define __OP32
+#elif defined(CONFIG_ARM_64)
+# define __OP32 "w"
+#else
+# error "unknown ARM variant"
+#endif
+
 #endif /* __ARM_ASM_DEFNS_H__ */
 /*
  * Local variables:
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -9,6 +9,8 @@
 #ifndef _ARM_BITOPS_H
 #define _ARM_BITOPS_H
 
+#include <asm/asm_defns.h>
+
 /*
  * Non-atomic bit manipulation.
  *
@@ -111,9 +113,8 @@ static inline int fls(unsigned int x)
         if (__builtin_constant_p(x))
                return generic_fls(x);
 
-        asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
-        ret = BITS_PER_LONG - ret;
-        return ret;
+        asm("clz\t%"__OP32"0, %"__OP32"1" : "=r" (ret) : "r" (x));
+        return 32 - ret;
 }
 
 
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -3,6 +3,9 @@
 
 #include <asm/cpregs.h>
 #include <asm/sysregs.h>
+#ifndef __ASSEMBLY__
+#include <xen/types.h>
+#endif
 #include <public/arch-arm.h>
 
 /* MIDR Main ID Register */
@@ -220,8 +223,6 @@
 
 #ifndef __ASSEMBLY__
 
-#include <xen/types.h>
-
 struct cpuinfo_arm {
     union {
         uint32_t bits;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

  parent reply	other threads:[~2015-01-22 13:38 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-22 13:26 [PATCH 0/2] fls() / ffs() adjustments Jan Beulich
2015-01-22 13:38 ` [PATCH 1/2] make fls() and ffs() consistent across architectures Jan Beulich
2015-01-23 14:38   ` Andrew Cooper
2015-01-23 14:53     ` Jan Beulich
2015-01-22 13:38 ` Jan Beulich [this message]
2015-01-22 14:01   ` [PATCH 2/2] arm64: fix fls() Tim Deegan
2015-01-22 14:05     ` Jan Beulich
2015-01-22 14:29       ` Tim Deegan
2015-01-22 14:59 ` [PATCH 0/2] fls() / ffs() adjustments Ian Campbell

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=54C10B7E0200007800058256@mail.emea.novell.com \
    --to=jbeulich@suse.com \
    --cc=Ian.Campbell@eu.citrix.com \
    --cc=stefano.stabellini@eu.citrix.com \
    --cc=tim@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /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: link
Be 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.