Thread: raw1394_request user/kernel space size differences
Brought to you by:
aeb,
bencollins
Menu
▾
▴
linux1394-devel
|
From: Ben C. <bco...@de...> - 2000-05-27 00:13:22
|
I'm messing around with getting the ieee1394 drivers working across platforms (specifically I am testing with sparc64). The unusual thing about sparc64 is that the user space is (currently) 32bit, while the kernel is 64bit. This makes sizeof(struct raw1394_request) different between the kernel driver and the libraw1394 library (56 in the driver, 40 in the library, mainly because unsigned int and size_t are 8 in the kernel and 4 in userspace). Now, usually this would be overcome in cases like this for sparc64 because the comunication is via ioctl's, and sparc64 has conversions. However, this is a different case since we are reading and writing directly to the device. I forced the size's in the library to match that of driver for each member of the struct, but that causes an oops with: Unsupported unaligned load/store trap for kernel testlibraw(13217): Kernel does fpu/atomic unaligned load/store. Any ideas on working around this? I suspect that libraw1394 will have to be special cased for this (which is a hack) but the driver code still performs this unaligned bit on 64bit (which I suspect is the real problem). The driver most likely needs to use things like u32 for the struct, and libraw1394 will need some configure.in macros for detecting properly sized replacements to match the kernel. I am going to start looking at it, but I wanted some other people closer to the code to realize this (mostly because there are probably other 64bit issues in the drivers aswell). Thanks, Ben -- -----------=======-=-======-=========-----------=====------------=-=------ / Ben Collins -- ...on that fantastic voyage... -- Debian GNU/Linux \ ` bco...@de... -- bco...@op... -- bco...@li... ' `---=========------=======-------------=-=-----=-===-======-------=--=---' |
|
From: Ben C. <bco...@de...> - 2000-05-27 05:12:30
|
Ok, I've talked to Dave Miller about this problem to some extent since he
is familiar with intermixed 32/64 environments. ia64, ppc64 and mips64
promise to offer the same problems, so it's obviously important to fix
this up now. Either one of two things can be done to fix the problem with
the pointers in the raw1394_request struct:
1) Convert to using ioctl's and have the archs that have 32/64 userspace do
translations (just as sparc64 does now in arch/sparc64/kernel/ioctl32.c)
2) The ATM code uses this (from include/linux/atmapi.h):
/*
* Opaque type for kernel pointers. Note that _ is never accessed. We need
* the struct in order hide the array, so that we can make simple assignments
* instead of being forced to use memcpy. It also improves error reporting for
* code that still assumes that we're passing unsigned longs.
*
* Convention: NULL pointers are passed as a field of all zeroes.
*/
typedef struct { unsigned char _[8]; } atm_kptr_t;
...to abstract the pointers. I'm not sure how it's used, but Dave pointed me
in this direction (albeit, he prefered the ioctl method). Either method
will most likely require changes in the API for /dev/raw1394, libraw1394
internals and some special code in the kernel for the 32/64 mixed archs.
Let me know which is prefered and I can start working to code the needed
changes if wanted.
--
-----------=======-=-======-=========-----------=====------------=-=------
/ Ben Collins -- ...on that fantastic voyage... -- Debian GNU/Linux \
` bco...@de... -- bco...@op... -- bco...@li... '
`---=========------=======-------------=-=-----=-===-======-------=--=---'
|
|
From: Andreas B. <and...@mu...> - 2000-05-28 21:02:23
|
On Sat, May 27, 2000 at 01:08:08AM -0400, Ben Collins wrote:
> 1) Convert to using ioctl's and have the archs that have 32/64 userspace do
> translations (just as sparc64 does now in arch/sparc64/kernel/ioctl32.c)
>
> 2) The ATM code uses this (from include/linux/atmapi.h):
>
> typedef struct { unsigned char _[8]; } atm_kptr_t;
>
> ...to abstract the pointers. I'm not sure how it's used, but Dave pointed me
> in this direction (albeit, he prefered the ioctl method). Either method
> will most likely require changes in the API for /dev/raw1394, libraw1394
> internals and some special code in the kernel for the 32/64 mixed archs.
>
> Let me know which is prefered and I can start working to code the needed
> changes if wanted.
I'd rather avoid ioctls and have the pointer fields in kernel
pointer size.
For that, an autoconf macro needs to be written which determines if
we're compiling for 32/64 environments and make those fields 64 bits
wide. Setting these fields cast from quadlet_t*. I don't think there
is an easy way to determine 32/64, we'd probably have to distinguish
by known architecture names.
[time passes...]
Actually, I have implemented a simple but effective fix now. Kernel
side doesn't change, libraw1394 casts pointers to kernel pointer width
if neccessary. For now configure requires --enable-32-64 on the
command line, there is no automated way to recognize a splitted
environment.
Update libraw1394 from CVS, run autoheader and autoconf at least (if
you have the other stuff lying around already), then
"./configure --enable-32-64", make and test it.
--
Andreas E. Bombe <and...@mu...> DSA key 0x04880A44
http://home.pages.de/~andreas.bombe/ http://linux1394.sourceforge.net/
|
|
From: Ben C. <bco...@de...> - 2000-05-29 00:43:32
|
> Actually, I have implemented a simple but effective fix now. Kernel > side doesn't change, libraw1394 casts pointers to kernel pointer width > if neccessary. For now configure requires --enable-32-64 on the > command line, there is no automated way to recognize a splitted > environment. This is very wrong. For example, I should be able to compile a 32bit sparc binary that runs on sun4cdm (32bit kernel arch) and sun4u (64bit kernel arch) just like any other binary does. This makes the resulting binary specific to the kernel, and this I wont have that kind of portability. This might be ok on a per user basis, but when libraw1394 hits the distributions (and it will be in Debian soon I suspect), it will play hell with packaging. ia64, ppc64 an mips64 will have the same problem. The 32bit binaries should be portable, and the "hackery" should be in the kernel so it is invisible to userspace. Take modutils for example. It is only compiled as 32bit, and the resulting binary can handle 32bit and 64bit modules. Ben -- -----------=======-=-======-=========-----------=====------------=-=------ / Ben Collins -- ...on that fantastic voyage... -- Debian GNU/Linux \ ` bco...@de... -- bco...@op... -- bco...@li... ' `---=========------=======-------------=-=-----=-===-======-------=--=---' |
|
From: Ben C. <bco...@de...> - 2000-05-29 02:24:23
|
> Update libraw1394 from CVS, run autoheader and autoconf at least (if > you have the other stuff lying around already), then > "./configure --enable-32-64", make and test it. Ok, I tested this and it worked. So I took it one step further. Attached are two patches. One for linux1394 CVS which defines a kptr_t just like you did for libraw1394. The other is for libraw1394 to sync the raw1394.h from the kernel source. Using this, you can completely remove the --enable-32-64 configure option. It makes the pointer massaging completely transparent, and should work for other 32/64 mixed env's besides sparc/sparc64. I tested this by compiling a 32bit and 64bit binary, and both worked perfectly as expected. Ben -- -----------=======-=-======-=========-----------=====------------=-=------ / Ben Collins -- ...on that fantastic voyage... -- Debian GNU/Linux \ ` bco...@de... -- bco...@op... -- bco...@li... ' `---=========------=======-------------=-=-----=-===-======-------=--=---' |
|
From: Ben C. <bco...@de...> - 2000-05-29 22:13:16
Attachments:
raw1394.diff
libraw1394.diff
|
> Ok, I tested this and it worked. So I took it one step further. Attached > are two patches. One for linux1394 CVS which defines a kptr_t just like > you did for libraw1394. The other is for libraw1394 to sync the raw1394.h > from the kernel source. Guess attaching the patches would actually help. -- -----------=======-=-======-=========-----------=====------------=-=------ / Ben Collins -- ...on that fantastic voyage... -- Debian GNU/Linux \ ` bco...@de... -- bco...@op... -- bco...@li... ' `---=========------=======-------------=-=-----=-===-======-------=--=---' |
|
From: Andreas B. <and...@mu...> - 2000-06-01 23:19:35
|
On Mon, May 29, 2000 at 05:45:35PM -0400, Ben Collins wrote:
> > Ok, I tested this and it worked. So I took it one step further. Attached
> > are two patches. One for linux1394 CVS which defines a kptr_t just like
> > you did for libraw1394. The other is for libraw1394 to sync the raw1394.h
> > from the kernel source.
>
> Guess attaching the patches would actually help.
>
>--
[...]
> struct raw1394_request {
> - int type;
> - int error;
> - int misc;
> -
> - unsigned int generation;
> - octlet_t address;
> -
> - unsigned long tag;
> -
> - size_t length;
> - quadlet_t *sendb;
> - quadlet_t *recvb;
> + __s32 type;
> + __s32 error;
> + __s32 misc;
> +
> + __u32 generation;
> + __u64 address;
> +
> + __u64 tag;
> +
> + __u32 length;
> +#ifdef __KERNEL__
> + raw1394_kptr_t sendb;
> + raw1394_kptr_t recvb;
> +#else
> +/* We use this to make it easy to pass pointers to the kernel,
> + * even in mixed 32/64 bit environments */
> +#define SET_CAST_PTR(x,p) \
> +if (sizeof(void *) == sizeof(__u32)) { \
> + x.large = 0; \
> + x.small[1] = (__u32)p; \
> +} else { \
> + x.large = (__u64)p; \
> +}
> +
> + union {
> + __u32 small[2];
> + __u64 large;
> + } sendb;
> + union {
> + __u32 small[2];
> + __u64 large;
> + } recvb;
> +#endif
> };
Using explicit width types for everything seems like a good idea. But
we can also just forget the environment and always use __u64 types for
the pointer fields. Writing a 64 bit field in a 32 bit environment
will clear the upper 32 and write the lower 32 bits, just like your
SET_CAST_PTR macro --- just cleaner and without endianness problems
(looking at your macro I can see Sparc is run big endian).
If we'd be really tightassed, we could make autoconf efforts to make
those types 32 bit on fully 32 bit platforms like x86, but zeroing a
few bytes really shouldn't be a performance problem.
I'll try that tomorrow.
--
Andreas E. Bombe <and...@mu...> DSA key 0x04880A44
http://home.pages.de/~andreas.bombe/ http://linux1394.sourceforge.net/
|
|
From: Andreas B. <and...@mu...> - 2000-06-02 17:09:40
|
On Fri, Jun 02, 2000 at 01:16:11AM +0200, Andreas Bombe wrote: > On Mon, May 29, 2000 at 05:45:35PM -0400, Ben Collins wrote: > > > Ok, I tested this and it worked. So I took it one step further. Attached > > > are two patches. One for linux1394 CVS which defines a kptr_t just like > > > you did for libraw1394. The other is for libraw1394 to sync the raw1394.h > > > from the kernel source. > > > > Guess attaching the patches would actually help. > > Using explicit width types for everything seems like a good idea. But > we can also just forget the environment and always use __u64 types for > the pointer fields. Writing a 64 bit field in a 32 bit environment > will clear the upper 32 and write the lower 32 bits, just like your > SET_CAST_PTR macro --- just cleaner and without endianness problems > (looking at your macro I can see Sparc is run big endian). I implemented that now and put it in CVS. This time you need to update both kernel and libraw1394. The compiler warnings (cast to/from pointer from/to int of different size) however annoy me. I'll see what I can do about that... -- Andreas E. Bombe <and...@mu...> DSA key 0x04880A44 http://home.pages.de/~andreas.bombe/ http://linux1394.sourceforge.net/ |
|
From: Daniel K. <dan...@st...> - 2000-06-02 17:45:36
|
On Fri, 2 Jun 2000, Andreas Bombe wrote: > I implemented that now and put it in CVS. This time you need to > update both kernel and libraw1394. By the way - how about setting a mailing list where CVS commit logs are sent to? We do it at the GLAME project, and I tend to find it terribly convenient. Regards, Daniel. -- GNU/Linux Audio Mechanics - http://www.glame.de GPG Key ID 89BF7E2B - http://www.keyserver.net |
Thanks for helping keep SourceForge clean.
X