| Message ID | 1383065529-20001-1-git-send-email-wcohen@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi William, On 29/10/13 16:52, William Cohen wrote: > The 32-bit ARM does not have instructions to perform 16-bit loads or > stores. The __get_user_asm_half and __put_user_asm_half macros > sythesize those operations. However, in most cases the pointers > passed into these macros are pointers to 16-bit types and the pointer > arithmetic will end up pointing at the next 16-bit quantity rather > than the second half (byte) of the 16-bit quantity. The macros need > to explicitly typecast the pointers as pointers to 8-bit quantities to > make the pointer arithmetic work out properly. So I'm a bit perplex here. Looks like nobody but __{ge,pu}t_user_err calls these macros directly, and the sole users cast the pointer to an unsigned long, for which the arithmetic ought to work properly. What am I missing? M. > Signed-off-by: William Cohen <wcohen@redhat.com> > --- > arch/arm/include/asm/uaccess.h | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h > index 7e1f760..c2d9439 100644 > --- a/arch/arm/include/asm/uaccess.h > +++ b/arch/arm/include/asm/uaccess.h > @@ -277,16 +277,16 @@ do { \ > #define __get_user_asm_half(x,__gu_addr,err) \ > ({ \ > unsigned long __b1, __b2; \ > - __get_user_asm_byte(__b1, __gu_addr, err); \ > - __get_user_asm_byte(__b2, __gu_addr + 1, err); \ > + __get_user_asm_byte(__b1, (u8 *)(__gu_addr), err); \ > + __get_user_asm_byte(__b2, ((u8 *)(__gu_addr)) + 1, err);\ > (x) = __b1 | (__b2 << 8); \ > }) > #else > #define __get_user_asm_half(x,__gu_addr,err) \ > ({ \ > unsigned long __b1, __b2; \ > - __get_user_asm_byte(__b1, __gu_addr, err); \ > - __get_user_asm_byte(__b2, __gu_addr + 1, err); \ > + __get_user_asm_byte(__b1, (u8 *)(__gu_addr), err); \ > + __get_user_asm_byte(__b2, ((u8 *)(__gu_addr)) + 1, err);\ > (x) = (__b1 << 8) | __b2; \ > }) > #endif > @@ -358,15 +358,15 @@ do { \ > #define __put_user_asm_half(x,__pu_addr,err) \ > ({ \ > unsigned long __temp = (unsigned long)(x); \ > - __put_user_asm_byte(__temp, __pu_addr, err); \ > - __put_user_asm_byte(__temp >> 8, __pu_addr + 1, err); \ > + __put_user_asm_byte(__temp, (u8 *)(__pu_addr), err); \ > + __put_user_asm_byte(__temp >> 8, ((u8 *)(__pu_addr)) + 1, err);\ > }) > #else > #define __put_user_asm_half(x,__pu_addr,err) \ > ({ \ > unsigned long __temp = (unsigned long)(x); \ > - __put_user_asm_byte(__temp >> 8, __pu_addr, err); \ > - __put_user_asm_byte(__temp, __pu_addr + 1, err); \ > + __put_user_asm_byte(__temp >> 8, (u8 *)(__pu_addr), err);\ > + __put_user_asm_byte(__temp, ((u8 *)(__pu_addr)) + 1, err);\ > }) > #endif > >
On Tue, Oct 29, 2013 at 12:52:09PM -0400, William Cohen wrote: > The 32-bit ARM does not have instructions to perform 16-bit loads or > stores. The __get_user_asm_half and __put_user_asm_half macros > sythesize those operations. However, in most cases the pointers > passed into these macros are pointers to 16-bit types and the pointer > arithmetic will end up pointing at the next 16-bit quantity rather > than the second half (byte) of the 16-bit quantity. The macros need > to explicitly typecast the pointers as pointers to 8-bit quantities to > make the pointer arithmetic work out properly. NAK. Nothing should be making use of these macros other than the other macros in that file; these are implementation specific helper macros. There, the address passed as __gu_addr and __pu_addr is an 'unsigned long' quantity, so there is no concern about pointer arithmetic; they do not take pointers.
On 10/29/2013 02:04 PM, Russell King - ARM Linux wrote: > On Tue, Oct 29, 2013 at 12:52:09PM -0400, William Cohen wrote: >> The 32-bit ARM does not have instructions to perform 16-bit loads or >> stores. The __get_user_asm_half and __put_user_asm_half macros >> sythesize those operations. However, in most cases the pointers >> passed into these macros are pointers to 16-bit types and the pointer >> arithmetic will end up pointing at the next 16-bit quantity rather >> than the second half (byte) of the 16-bit quantity. The macros need >> to explicitly typecast the pointers as pointers to 8-bit quantities to >> make the pointer arithmetic work out properly. > > NAK. > > Nothing should be making use of these macros other than the other > macros in that file; these are implementation specific helper macros. > There, the address passed as __gu_addr and __pu_addr is an 'unsigned > long' quantity, so there is no concern about pointer arithmetic; they > do not take pointers. > You are correct. systemtap had had macros that were based on the arm uaccess.h macos, but they were missing the typecasts that __get_user_err and __put_user_err. Thanks for pointing out my error. -Will
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h index 7e1f760..c2d9439 100644 --- a/arch/arm/include/asm/uaccess.h +++ b/arch/arm/include/asm/uaccess.h @@ -277,16 +277,16 @@ do { \ #define __get_user_asm_half(x,__gu_addr,err) \ ({ \ unsigned long __b1, __b2; \ - __get_user_asm_byte(__b1, __gu_addr, err); \ - __get_user_asm_byte(__b2, __gu_addr + 1, err); \ + __get_user_asm_byte(__b1, (u8 *)(__gu_addr), err); \ + __get_user_asm_byte(__b2, ((u8 *)(__gu_addr)) + 1, err);\ (x) = __b1 | (__b2 << 8); \ }) #else #define __get_user_asm_half(x,__gu_addr,err) \ ({ \ unsigned long __b1, __b2; \ - __get_user_asm_byte(__b1, __gu_addr, err); \ - __get_user_asm_byte(__b2, __gu_addr + 1, err); \ + __get_user_asm_byte(__b1, (u8 *)(__gu_addr), err); \ + __get_user_asm_byte(__b2, ((u8 *)(__gu_addr)) + 1, err);\ (x) = (__b1 << 8) | __b2; \ }) #endif @@ -358,15 +358,15 @@ do { \ #define __put_user_asm_half(x,__pu_addr,err) \ ({ \ unsigned long __temp = (unsigned long)(x); \ - __put_user_asm_byte(__temp, __pu_addr, err); \ - __put_user_asm_byte(__temp >> 8, __pu_addr + 1, err); \ + __put_user_asm_byte(__temp, (u8 *)(__pu_addr), err); \ + __put_user_asm_byte(__temp >> 8, ((u8 *)(__pu_addr)) + 1, err);\ }) #else #define __put_user_asm_half(x,__pu_addr,err) \ ({ \ unsigned long __temp = (unsigned long)(x); \ - __put_user_asm_byte(__temp >> 8, __pu_addr, err); \ - __put_user_asm_byte(__temp, __pu_addr + 1, err); \ + __put_user_asm_byte(__temp >> 8, (u8 *)(__pu_addr), err);\ + __put_user_asm_byte(__temp, ((u8 *)(__pu_addr)) + 1, err);\ }) #endif
The 32-bit ARM does not have instructions to perform 16-bit loads or stores. The __get_user_asm_half and __put_user_asm_half macros sythesize those operations. However, in most cases the pointers passed into these macros are pointers to 16-bit types and the pointer arithmetic will end up pointing at the next 16-bit quantity rather than the second half (byte) of the 16-bit quantity. The macros need to explicitly typecast the pointers as pointers to 8-bit quantities to make the pointer arithmetic work out properly. Signed-off-by: William Cohen <wcohen@redhat.com> --- arch/arm/include/asm/uaccess.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)