Re: paddr_t change to 64-bits

[Iain Hibbert <plunky%rya-online.net@localhost>]

On Mon, 27 Sep 2010, Christos Zoulas wrote:

> >> struct agp_allocate from <sys/agpio.h> contains paddr_t and it is the
> >> definition of paddr_t which is broken because <i386/types.h> now defines
> >> it differently for non-PAE kernel (32-bit) and userland (64-bit).
> >
> >so? Define AGPIOC_ALLOCATE2, and patch the drm code to use it?
> >
> >AGPIOC_ALLOCATE => old code, assume 32 bits paddr_t as interface (old
> >i386 interface). Kernel will convert to 32 bits depending on its i386 mode.
> >
> >AGPIOC_ALLOCATE2 => new code, assume 64 bits paddr_t as interface.
> >Kernel code will pass a 64 bits paddr_t for physical.
> >
> >Opinions?
>
> I've already fixed it. I am not sure what to do in the PAE case though.

Alas your fix doesn't work. As mentioned elsewhere there is a build
failure in the non-PAE case. But also, for new userland binaries, the
structure passed contains a 64-bit paddr_t but the kernel is expecting
32-bit so the compat translation needs to be the other way around..

Then, values need to be returned.. the patch attached works for me with
new binaries, but I only have an i386/non-PAE system here.

iain

Index: agp.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/agp.c,v
retrieving revision 1.71
diff -u -r1.71 agp.c
--- agp.c       27 Sep 2010 22:53:46 -0000      1.71
+++ agp.c       28 Sep 2010 11:36:22 -0000
@@ -944,34 +944,40 @@
        case AGPIOC_SETUP:
                return agp_setup_user(sc, (agp_setup *)data);
 
-#ifdef __i386__
+#if defined(__i386__) && !defined(PAE)
 {
        /*
-        * Handle paddr_t change from 32 bit for non PAE kernels
-        * to 64 bit.
+        * userland passes a 64-bit paddr_t but non PAE kernels
+        * are still expecting a 32-bit value here
         */
-#define AGPIOC_OALLOCATE  _IOWR(AGPIOC_BASE, 6, agp_oallocate)
+#define AGPIOC_ALLOCATE64  _IOWR(AGPIOC_BASE, 6, agp_allocate64)
 
-       typedef struct _agp_oallocate {
+       typedef struct _agp_allocate64 {
                int key;                /* tag of allocation            */
                size_t pg_count;        /* number of pages              */
                uint32_t type;          /* 0 == normal, other devspec   */
-               u_long physical;        /* device specific (some devices
+               uint64_t physical;      /* device specific (some devices
                                         * need a phys address of the
                                         * actual page behind the gatt
                                         * table)                        */
-       } agp_oallocate;
+       } agp_allocate64;
 
-       case AGPIOC_OALLOCATE: {
+       case AGPIOC_ALLOCATE64: {
                agp_allocate aga;
-               agp_oallocate *oaga = data;
+               agp_allocate64 *aga64 = data;
+               int error;
 
-               aga.key = oaga->key;
-               aga.pg_count = oaga->pg_count;
-               aga.type = oaga->type;
-               aga.physical = oaga->physical;
-
-               return agp_allocate_user(sc, &aga);
+               aga.key = aga64->key;
+               aga.pg_count = aga64->pg_count;
+               aga.type = aga64->type;
+               aga.physical = (paddr_t)aga64->physical;
+
+               error = agp_allocate_user(sc, &aga);
+               if (error == 0) {
+                       aga64->key = aga.key;
+                       aga64->physical = (uint64_t)aga.physical;
+               }
+               return error;
        }
 }
 #endif