Bug 943

Created attachment 540 [details] [review]
Patch described in previous comment.

Created attachment 541 [details] [review]
Oops. Fixed patch

Previous patch was missing pieces. Sorry.

*** Bug 942 has been marked as a duplicate of this bug. ***

Reassigning to dri-devel.

Comments?

I expect that the code no longer applies to the current CVS version.
I guess I need to port it again. Furthermore I was going to look into
making the backward compatibility handling cleaner.

The big issue is backwards compatiblity, this patch needs to be developed as two
separate patches, one for the kernel, then tested with current X releases and
submitted via the DRM tree, then one for Mesa and X that can also work with the
old drm on normal systems (this isn't so important, you can get users to upgrade
their kernel if they are upgrading X just use the DRM versioning, but upgrading
your kernel should never break your X/DRI)...

I also meant to say that if for some reason this is impossible, then these
reasons should be raised with the kernel/X people and we find the solution that
breaks the least number of working systems.... 

My belief at the moment (with no 64-bit machines to validate anything on) is
that anything we do will probably break pure 64-bit clients, breaking pure
32-bit clients is of course completely unacceptable....  but maybe someone can
find a way...

Any updates on this yet ? 
A patch against the new drm would be fine. 
Greetings Jan 

I haven't had time to do this, yet.
It's on my list.

Created attachment 2011 [details] [review]
Patch to add ioctl compatibility routines for 32-bit processes

Here is my attempt at implementing ioctl compatibility routines for 32-bit
processes on a 64-bit kernel.  The patch is against a 2.6.11 Linux kernel tree.
 I have done the core DRM and the Radeon driver but not any of the other
chipset drivers.

I haven't changed the ABI, either for 32-bit or for 64-bit processes.  I have
used this successfully on a G5 powermac running a ppc64 Linux kernel, using
unmodified 32-bit X server and client binaries.

Hi
i used the patch by Paul Mackerras on an x86-64 machine running Debian-Sid with
Xorg from Ubuntu (with latest xorg-cvs ati drivers) and r300_driver from
r300.sf.net. I tested it on an Pure ia32 setup and on my x86-64 setup. I tested
with glxgears :) and neverball. It works for me in an pure ia32-setup, in an
pure x86-64 setup. It also works when i boot an x86-64 kernel and chroot into my
ia32-setup (as i think this is similiar to Pauls setup this was to be expspected).
However when i boot into x86-64 and start and x86-64 Xserver and then chroot
into my ia32-setup every gl-related operation (glxinfo, neverball ...) segfault.
It would be nice, if i could start an x86-64 Xserver and use 32-bit gl-apps (as
it is possible with fglrx), however currently i am very happy with the solution
to stop my x86-64 Xserver, chroot into my ia32-setup and start the ia32 Xserver
(as this was mostly missing using dri on x86-64). If you need more Information
(eg on the segfault, or an patch against r300.sf.net drm sources or ...) feel
free to ask.
Thanks for your work.

cheers Jan Kreuzer

Created attachment 2024 [details]
dmesg from 64-bit kernel and Xserver and 32-bit glxinfo

This is my dmesg output from an X86-64 kernel and Xserver and an ia32 chrooted
glxinfo.

Created attachment 2025 [details]
strace from 32-bit glxinfo on 64-bit kernel and Xserver

This is the strace from an chrooted ia32 glxinfo on an X86-64 Kernel and
Xserver.

Hmmm, presumably the X server is doing the addmap for the SAREA and then the
client is mmapping it, but is using the wrong offset value.  I see that the
SAREA handle gets sent from the X server to the client in XF86DRIOpenConnection.
 I think the way to solve the problem is to always make a 32-bit handle for
_DRM_SHM memory when CONFIG_COMPAT is defined, so that the X server will get a
32-bit handle for the SAREA even if it is a 64-bit process.

> I think the way to solve the problem is to always make a 32-bit handle for
> _DRM_SHM memory when CONFIG_COMPAT is defined, so that the X server will get a
> 32-bit handle for the SAREA even if it is a 64-bit process.

That's exactly what my code is doing.
I've got an updated version of my patch that handles backward compatibility much
more gracefully.

In comment #11 Paul Mackerras writes:

> I haven't changed the ABI, either for 32-bit or for 64-bit processes.  I have
> used this successfully on a G5 powermac running a ppc64 Linux kernel, using
> unmodified 32-bit X server and client binaries.

Unfortunately without a change in the ABI it is not possible to run 32bit
DRI clients on 64 bit servers (or vice versa). The problem is that data
structures shared between kernel, Xserver and clients have different sizes.
However it is conceivable that users may want to run 32 and 64 applications on
the same Xserver. 
With the reight defines the ioctl converters are very boiler plate.
I'm going to attach an updated version of my patch. Presently I have support to
the MGA, Radeon and Rage128 driver. 
The changes to DRM should be backward compatible so that unpatched Xservers and
DRI libraries continue to work.
Xserver, libGL and DRI drivers need to be updated together however.

Created attachment 2362 [details] [review]
Updated patch (DRM part)

This patch changes DRM so that 32bit DRI clients work on a 64bit kernel. It
should be backward compatible to older versions of Xservers and client
libraries.

Created attachment 2363 [details] [review]
Fix for Mesa DRI drivers.

This patch fixes the Mesa DRI drivers to support 32bit DRI clients on 64bit
systems. It requires the patch in attachment #2362 [details] [review] and the Xserver patch that
is going to be attached.

Created attachment 2364 [details] [review]
Fix for Xserver part.

This patch makes the required adjustments to the Xserver that are required when
using attachment #2362 [details] [review] and #2363.
The patch applies to a recent version of X.Org head. I have not bothered to
apply all patches from #2363 to extra/Mesa also.

I still don't see any reason why the ABI needs to be changed.  The comment
"data structures shared between kernel, Xserver and clients have different
sizes" is too vague to be useful - which data structures, and what stops them
being converted?

> I still don't see any reason why the ABI needs to be changed.  The comment
> "data structures shared between kernel, Xserver and clients have different
> sizes" is too vague to be useful - which data structures, and what stops them
> being converted?
Let me give you two examples:
1. mga_dri.h MGADRIRec is a data structure that is private to the Matrox DRI
   driver. It gets initialized in the DRI X driver. 
   A DRI client (better to say the dri client module) can obtain this data 
   structure from the Xserver thru the DRI extension request 
   X_XF86DRIGetDeviceInfo.
   Usually X Request structures are carefully designed to be machine 
   independent. Not in this case however: Since DRI assumes that communication 
   happens locally it simply copies over the entire structure.
   If you look at the above structure it contains drmRegion elements.
   drmRegion contains a pointer and this is 32bit on a 32bit system or for a 
   32bit client on a 64bit system and 64bit if the binary is 64 bit.
   Therefore the last element of the MGADRIRec sarea_priv_offset comes at a   
   different offset depending if your client is 32 or 64 bit. If you connect
   to a 64bit Xserver from a 32bit client your sarea_priv_offset value read by 
   the client is probably wrong. 
   Why should do the conversion? There is no way for the client to know if the  
   structure has been sent by a 32bit or 64bit server. Also the server doesn't  
   know what client is connected. (Only endianess information is transmitted in 
   the X connection block).
2. The SAREA also contains a driver private structure (in case of the matrox 
   driver contained in mga_sarea.h:MGASAREAPrivRec 
   (drm: mga_drm.h:drm_mga_sarea_t). Its data is shared between the kernel and a 
   DRI client. On 64bit systems it's conceivable that the kernel runs 64bit.
   How can the kernel and a DRI client share data thru a structure with   
   different size?
Converting the data structures of the ioctls is the easy part. You can simply
write 'wrappers'.
The only exception is where the kernel hands over 64bit 'handles' to user space
which are used as offsets in mmap(). This doesn't work in 32bit user space.
I've solved this problem without changing the ABI. However the handle is no real
pointer any more which could cause issues to some drivers.

On the DRM portion can you look at the work Paul has done, it is much cleaner
and less intrusive, you are also using the old method which is deprecated now in
the kernel, the new method is available in Pauls patch in the -mm kernel, maybe
Paul can generate a patch against 2.6.11....

My feeling on this now is that we should change to generating hashes for SHM
handles for *all* platforms, having it different on 32 and 64 bits isn't going
to help thing, make it break obviously for everyone and we'll find out if we can
use this method or not ...

I'd like to merge the DRM into the kernel and see who complains and maybe
provide a command line option to fall back to the old 32-bit handle for 32-bit
systems in case somebody breaks badly...

I think between the two of you a clean solution should be possible,
breaking the ABI between X and DRI clients isn't as bad as breaking the
kernel/userspace boundard but I'd like to keep the 32-bit ABI the same if
possible but I guess it might not be, I've no issues with telling someone to
upgrade X and DRI at the same time from snapshots..  

After a chat on IRC perhaps we can define a new DRIRec and just up the DDX
numbers for the affected drivers.. then clients can know new/old DDX and either
just use the new structure or do either of fail/fixup for the old structure
depending on what they are running on.

OK, Egbert is right concerning the server -> client communication.  I ran across
this with RADEONDRIRec.  I checked all the SAREA definitions and they all seem
to be OK, fortunately.

It may be possible to convert in the client (based on the size of the structure
received from the server) so that a new client can run with either a 32-bit or
64-bit server.  Longer term I agree that changing the RADEONDRIRec and other
structures to be wordsize-independent is a good idea.

One could do a conversion - on the other hand a brakage of the ABI compatibility
between client and Xserver would not be all that bad - given that we cannot
really distinguish if a 32bit client library contains code that fixes the size
or not.
The main issue that I ran into was the size of drm_handle_t which I changed from
'long' to 'int'. It's defined in the drm headers however the drm modules don't
seem to use it at all.
It ensures that the same handles are used in 32 and 64bit and can be exchanged 
between 32 and 64bit pieces. This is done between the Xserver and 32bit clients.
This however requires that 32 and 64bit both return 32bit handles. This may be
the reason why my code is more intrusive.
Apart from this of all the drivers I fixed I only saw an issue with Matrox.

> On the DRM portion can you look at the work Paul has done, it is much cleaner
> and less intrusive, you are also using the old method which is deprecated now > in
> the kernel, the new method is available in Pauls patch in the -mm kernel,
> maybe
> Paul can generate a patch against 2.6.11....

The kernel ABI really hasn't changed. The ioctls have remained the same. The
only change I've made was to make sure the handles passed between kernel and
user space fit into 32bit.
 That means that they are not necessarily real offset values any more. If
anything needs real offset values it needs to obtain it thru another way. If fit
in 32bits my code attempts to reuse them. If they are 64bit however a 32bit hash
is created. It is also make sure that the handles are unique.
For your argument about intrusiveness please see above.

I'm presently reworking my driver to support both the old and the new method.
My intention was to make the creation of 32bit ioctls as boiler plate as
possible. My hope was that this would encourage people to provide compat support
  for every driver.

Created attachment 2418 [details] [review]
Patch for DRI client / X server compatibility

This is what I am using at the moment, and with this, both 32-bit and 64-bit
clients can connect to either a 32-bit or a 64-bit X server and do direct
rendering.  I have tested the r300 part of this patch on my G5.  I have made
the same changes to radeon and r200 and compile-tested them, but I don't have a
64-bit machine with a r100 or r200 graphics card.

I agree that the drm_handle_t is the main problem.  As for changing the
interface, I think we should do that when the next reasonable opportunity
arises.  But for now, something like this would let us fix the 32/64 problem
without breaking anything that currently works.

A comment w.r.t. generating hashed handles: there are some cases where the
radeon family of client-side DRI drivers do arithmetic to generate handles.  I
*think* this is only for AGP memory.  I have a suspicion that the X server may
also generate handles for the registers and/or the framebuffer by reading the
device's PCI bus address registers, but I would need to verify that.

Anyway, the point is that the handles can't be completely arbitrary for all
types of mappings, at least not without checking a lot of code...

> A comment w.r.t. generating hashed handles: there are some cases where the
> radeon family of client-side DRI drivers do arithmetic to generate handles.  I
> *think* this is only for AGP memory.  I have a suspicion that the X server may
> also generate handles for the registers and/or the framebuffer by reading the
> device's PCI bus address registers, but I would need to verify that.

This is totally broken behavior. It is very poor design. I have encountered such
cases (but I don't think the Radeon driver contained any of them - however the
MGA driver did.) I fixed all of them. Fortunately there were not too many.
There are better ways to obtain the information that is needed than to rely on
the handle to contain a certain piece of information.
> Anyway, the point is that the handles can't be completely arbitrary for all
> types of mappings, at least not without checking a lot of code...
Well, for some mappings we may be able to pass on the old values (if they fit
into 32 bit) This is what my latest code does. It still produces ambiguities
which need to be resolved.
I'm going to attach a patch to my latest version of the code that addresses some
security problems. 
The code can be compied to use either the old or new ioctl32 sceme.

Created attachment 2689 [details] [review]
Patch on top of 2362

This patch adds support for both the old and the new ioctl32 sceme and fixes a
security issue, a potential (yet unlikely) race condition.
Furthermore it attempts to return handles that fit into 32bit unmodified.
Client side code that uses the returned values for calculations should be fixed
- nevertheless this change should work around these problems.
This code has been tested with Radeon (in a modified tree as CVS head doesn't
work on my cards), R128 and Matrox.

Created attachment 2691 [details] [review]
Full patch (combining attachment #2362 [details] [review] and #2689)

To build the code without using compat_alloc_user_space() add the define
OLD_COMPAT.

Code is now in DRM CVS. Closing.