[SOLVED] Split addresses on 32 and 64 bit platforms

Julian Lewis · 02-26-2013, 11:01 AM

This has to run of 32 and 64 bit kernels.
Anyone know how to get rid of this error on 32 bit platforms

/vd80Drvr.c: In function ‘split_address’:
/vd80Drvr.c:33: warning: right shift count >= width of type

/**
* ===========================================================
* @brief Given a pointer split it into upr and lwr parts
* @param upr address where upper part of arddress will be stored
* @param lwr address where lower part of arddress will be stored
* @param ptr is the given pointer value to be split
*/

void split_address(uint32_t *upr, uint32_t *lwr, uintptr_t ptr)
{
*upr = (uint32_t) (ptr >> 32);
*lwr = (uint32_t) (ptr & 0xFFFFFFFF);
}

NevemTeve · 02-26-2013, 11:40 AM

Code:

 *upr = (uint32_t)(((uint64_t)ptr) >> 32);

mina86 · 02-26-2013, 04:46 PM

Code:

if (sizeof ptr == sizeof *lwr) {
    *lwr = (uint32_t)ptr;
    *uwr = 0;
} else {
    … what you have …
}

Julian Lewis · 02-27-2013, 02:26 AM

Thanks,
*upr = (uint32_t)(((uint64_t)ptr) >> 32);

I hope this code does not actually copy ptr to a long long on a 32bit platform

mina86 · 02-27-2013, 08:00 AM

Quote:

Originally Posted by Julian Lewis

I hope this code does not actually copy ptr to a long long on a 32bit platform

Why won't you check for yourself?

Code:

$ cat a.c
#include <stdint.h>

void split1(uint32_t *upr, uint32_t *lwr, uintptr_t ptr) {
	*upr = (uint32_t)((uint64_t)ptr >> 32);
	*lwr = (uint32_t)ptr;
}

void split2(uint32_t *upr, uint32_t *lwr, uintptr_t ptr) {
	if (sizeof ptr == sizeof *lwr) {
		*upr = 0;
	} else {
		*upr = (uint32_t)(ptr >> 32);
	}
	*lwr = (uint32_t)ptr;
}

void split3(uint32_t *upr, uint32_t *lwr, uint32_t ptr) {
	*upr = (uint32_t)((uint64_t)ptr >> 32);
	*lwr = (uint32_t)ptr;
}
$ gcc -O2 -o a.S -S a.c
$ cat a.S
	.file	"a.c"
	.text
	.p2align 4,,15
	.globl	split1
	.type	split1, @function
split1:
.LFB0:
	.cfi_startproc
	movq	%rdx, %rax
	shrq	$32, %rax
	movl	%eax, (%rdi)
	movl	%edx, (%rsi)
	ret
	.cfi_endproc
.LFE0:
	.size	split1, .-split1
	.p2align 4,,15
	.globl	split2
	.type	split2, @function
split2:
.LFB1:
	.cfi_startproc
	movq	%rdx, %rax
	shrq	$32, %rax
	movl	%eax, (%rdi)
	movl	%edx, (%rsi)
	ret
	.cfi_endproc
.LFE1:
	.size	split2, .-split2
	.p2align 4,,15
	.globl	split3
	.type	split3, @function
split3:
.LFB2:
	.cfi_startproc
	movl	$0, (%rdi)
	movl	%edx, (%rsi)
	ret
	.cfi_endproc
.LFE2:
	.size	split3, .-split3
	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
	.section	.note.GNU-stack,"",@progbits

Repeating for different optimisation flags is left as exercise for the reader.

Julian Lewis · 02-27-2013, 08:27 AM

Hi mina86,
that is very interesting, infact I am being told that uintptr_t is not used in the kernel, perhaps your example illustrates why.
Thanks

NevemTeve · 02-27-2013, 09:09 AM

Off: then please tell me why... if for some reason you have to treat a pointer as an integer, [u]intptr_t is the natural choice (or ptrdiff_t)

Julian Lewis · 02-27-2013, 09:13 AM

On a 32bit platform this is what you get.
I need to program a DMA transfer on a tsi148 VME bridge PCI chip
It has two registers upr and lwr

Code:
.file "a.c"
.text
.p2align 4,,15
.globl split1
.type split1, @function
split1:
pushl %ebp
xorl %edx, %edx
movl %esp, %ebp
pushl %ebx
movl 16(%ebp), %ebx
movl 8(%ebp), %ecx
movl %ebx, %eax
movl %edx, %eax
movl %eax, (%ecx)
movl 12(%ebp), %eax
xorl %edx, %edx
movl %ebx, (%eax)
popl %ebx
popl %ebp
ret
.size split1, .-split1
.p2align 4,,15
.globl split2
.type split2, @function
split2:
pushl %ebp
movl %esp, %ebp
movl 8(%ebp), %eax
movl $0, (%eax)
movl 12(%ebp), %eax
movl 16(%ebp), %edx
movl %edx, (%eax)
popl %ebp
ret
.size split2, .-split2
.p2align 4,,15
.globl split3
.type split3, @function
split3:
pushl %ebp
xorl %edx, %edx
movl %esp, %ebp
pushl %ebx
movl 16(%ebp), %ebx
movl 8(%ebp), %ecx
movl %ebx, %eax
movl %edx, %eax
movl %eax, (%ecx)
movl 12(%ebp), %eax
xorl %edx, %edx
movl %ebx, (%eax)
popl %ebx
popl %ebp
ret
.size split3, .-split3
.ident "GCC: (GNU) 4.1.2 20080704 (Red Hat 4.1.2-54)"
.section .note.GNU-stack,"",@progbits

NevemTeve · 02-27-2013, 09:57 AM

You should check how to set 'upr' in 32-bit mode. You might or might not have to do sign-extension, eg 87654321 -> FFFFFFFF87654321

mina86 · 02-27-2013, 10:22 AM

If you are writing a Linux driver and need to set up DMA you should use DMA API instead of whatever you seem to be doing. For more information see chapter 15 of LDD3, Documentation/DMA-API.txt and Documentation/DMA-API-HOWTO.txt.

Quote:

Originally Posted by Julian Lewis

On a 32bit platform this is what you get.

I bet any kind of optimisation will get rid of the differences, but it seems that my solution is best if no optimisation is enabled. Still it does not get rid of the warning as I hoped it would. So I guess it's either casting up or preprocessor:

Code:

void split_address(uint32_t *upr, uint32_t *lwr, uintptr_t ptr) {
#if UINTPTR_MAX == UINT32_MAX
    *upr = 0;  /* note NevemTeve comment though */
#else
    *upr = ptr >> 32;
#endif
    *lwr = ptr;
}

Quote:

Originally Posted by NevemTeve

Off: then please tell me why... if for some reason you have to treat a pointer as an integer, [u]intptr_t is the natural choice (or ptrdiff_t)

In Linux kernel, long or unsigned long is usually used if pointer needs to be stored as an integer. It so happens, that on all architectures Linux runs on, long can store a pointer.

Julian Lewis · 02-27-2013, 10:28 AM

ptrdiff_t is supposed to contain the difference between two pointers. It can even have negative values.
Apparently this type has been misused as a place to store pointers, but actually it is not big enough to hold a full 64 bit address.
uintptr_t is defined simply as unsigned long in linux/types.h and there is a load of bullshit declared around it in stdint.h
effectivley uintprt_t can always hold a full 64 bit pointer unlike ptrdiff_t and uintptr_t is the same as unsigned long.
What I like about uintptr_t is that you declare that you intend it to hold pointers when you use it, although the compiler will not complain if you store any other type in it.
Cheers

Julian Lewis · 02-27-2013, 10:33 AM

This is an email I got from someone who should know. In this case I am going to ignore his advice because as mentioned uintptr_t is just unsigned long.

Quote Email:

uintptr_t is never used in the kernel. It's an awful perverse thing.
The rule is that "unsigned long" is the type for addresses that must
not be dereferenced.

morgana% git grep -w uintptr_t | wc -l
426

The figure above is actually zero:

morgana% git grep unsigned.long | wc -l
106456

johnsfine · 02-27-2013, 10:42 AM

Quote:

Originally Posted by Julian Lewis

On a 32bit platform this is what you get.
I need to program a DMA transfer on a tsi148 VME bridge PCI chip
It has two registers upr and lwr

Do you expect to support 32-bit PAE?

I don't know how you translated whatever virtual address you had to a physical address, but in 32-bit PAE a physical address is 36-bits, and thus could not have been stored in a pointer.

I don't know what kernel support exists to help drivers with DMA (so the driver doesn't need to special case things like PAE), but that seems to be at least mentioned earlier in this thread. In general (though not in detail) I know drivers are not supposed to handle such details themselves, but are supposed to use general purpose constructs.

NevemTeve · 02-27-2013, 11:25 AM

> ptrdiff_t is supposed to contain the difference between two pointers.
True

> It can even have negative values.
Certainly.

> Apparently this type has been misused as a place to store pointers, but actually it is not big enough to hold a full 64 bit address.
Why do you think so? In 64-bit mode it has to be 64-bit long, to be able to store the difference of any two pointers.

> uintptr_t is defined simply as unsigned long in linux/types.h and there is a load of bullshit declared around it in stdint.h
Those 'pieces of bullshit' might be useful for those who want to write portable C-code. (eg in LLP64 intptr_t is 'long long', not 'long')

> effectively uintprt_t can always hold a full 64 bit pointer unlike ptrdiff_t and uintptr_t is the same as unsigned long.
It isn't the case in 32-bit mode: both ptrdiff_t and inptr_t are 32-bit long.

> What I like about uintptr_t is that you declare that you intend it to hold pointers when you use it, although the compiler will not complain if you store any other type in it.
Very true.

Julian Lewis · 03-01-2013, 02:52 AM

http://lkml.indiana.edu/hypermail/li...08.2/1018.html

Use of ptrdiff_t in

- if (!access_ok(VERIFY_WRITE, u_tmp->rx_buf, u_tmp->len))
+ if (!access_ok(VERIFY_WRITE, (u8 __user *)
+ (ptrdiff_t) u_tmp->rx_buf,
+ u_tmp->len))

is wrong; for one thing, it's a bad C (it's what uintptr_t is for; in general
we are not even promised that ptrdiff_t is large enough to hold a pointer,
just enough to hold a difference between two pointers within the same object).
For another, it confuses the fsck out of sparse.

Use unsigned long or uintptr_t instead. There are several places misusing
ptrdiff_t (one - in a very odd way; WTF did that cast to __user ptrdiff_t
in ntfs expect to happen, anyway?) Fixed...

Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>