Hi all,
This set of patches will clean-up and fix some of the issues that arise
with the current binder interface when moving to a 64bit kernel. All these
changes will not affect the existing 32bit Android interface and are meant
to stand as the base for the 64bit binder compat layer(kernel or userpsace).
The patch set has been successfully tested with a 64bit Linux userspace and
64bit binder unit-tests.
This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
kernel binder compat layer.
Changes for v3:
1: Dropped the patch that was replacing uint32_t types with unsigned int
2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's
staging tree)
3: Split one patch into two: 'modify binder_write_read' and '64bit changes'
4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
5: Modified the binder command IOCTL declarations according to Arve's review
Changes for v2:
1: 1/7: Modified the commit message according to Greg's feedback;
2: 3/7: Merged with the patch fixing the printk format specifiers.
Serban Constantinescu (6):
staging: android: binder: modify struct binder_write_read to use
size_t
staging: android: binder: fix binder interface for 64bit compat layer
staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
staging: android: binder: fix alignment issues
staging: android: binder: replace types with portable ones
drivers/staging/android/binder.c | 40 ++++++++++++++++-----------------
drivers/staging/android/binder.h | 46 +++++++++++++++++++-------------------
2 files changed, 43 insertions(+), 43 deletions(-)
--
1.7.9.5
This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
instead of size_t for setting the max threads. Thus using the same
handler for 32 and 64bit kernels.
This value is stored internally in struct binder_proc and set to 15
on open_binder() in the libbinder API(thus no need for a 64bit size_t
on 64bit platforms).
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 2f94d16..1761541 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -86,7 +86,7 @@ struct binder_version {
#define BINDER_WRITE_READ _IOWR('b', 1, struct binder_write_read)
#define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
-#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
+#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
#define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
#define BINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
#define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
--
1.7.9.5
Since this driver is meant to be used on different types of processors
and a portable driver should specify the size a variable expects to be
this patch changes the types used throughout the binder interface.
We use "userspace" types since this header will be exported and used by
the Android filesystem.
The patch does not change in any way the functionality of the binder driver.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.h | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index c3562c4..bff1c74 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -123,10 +123,10 @@ struct binder_transaction_data {
void *ptr; /* target descriptor of return transaction */
} target;
void *cookie; /* target object cookie */
- unsigned int code; /* transaction command */
+ __u32 code; /* transaction command */
/* General information about the transaction. */
- unsigned int flags;
+ __u32 flags;
pid_t sender_pid;
uid_t sender_euid;
size_t data_size; /* number of bytes of data */
@@ -143,7 +143,7 @@ struct binder_transaction_data {
/* offsets from buffer to flat_binder_object structs */
const void __user *offsets;
} ptr;
- uint8_t buf[8];
+ __u8 buf[8];
} data;
};
@@ -153,18 +153,18 @@ struct binder_ptr_cookie {
};
struct binder_pri_desc {
- int priority;
- int desc;
+ __s32 priority;
+ __s32 desc;
};
struct binder_pri_ptr_cookie {
- int priority;
+ __s32 priority;
void *ptr;
void *cookie;
};
enum binder_driver_return_protocol {
- BR_ERROR = _IOR('r', 0, int),
+ BR_ERROR = _IOR('r', 0, __s32),
/*
* int: error code
*/
@@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
* binder_transaction_data: the received command.
*/
- BR_ACQUIRE_RESULT = _IOR('r', 4, int),
+ BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
/*
* not currently supported
* int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
@@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
* binder_transaction_data: the sent command.
*/
- BC_ACQUIRE_RESULT = _IOW('c', 2, int),
+ BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
/*
* not currently supported
* int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
@@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
* void *: ptr to transaction data received on a read
*/
- BC_INCREFS = _IOW('c', 4, int),
- BC_ACQUIRE = _IOW('c', 5, int),
- BC_RELEASE = _IOW('c', 6, int),
- BC_DECREFS = _IOW('c', 7, int),
+ BC_INCREFS = _IOW('c', 4, __u32),
+ BC_ACQUIRE = _IOW('c', 5, __u32),
+ BC_RELEASE = _IOW('c', 6, __u32),
+ BC_DECREFS = _IOW('c', 7, __u32),
/*
* int: descriptor
*/
--
1.7.9.5
The changes in this patch will fix the binder interface for use on 64bit
machines and stand as the base of the 64bit compat support. The changes
apply to the structures that are passed between the kernel and
userspace.
Most of the changes applied mirror the change to struct binder_version
where there is no need for a 64bit wide protocol_version(on 64bit
machines). The change inlines with the existing 32bit userspace(the
structure has the same size) and simplifies the compat layer such that
the same handler can service the BINDER_VERSION ioctl.
Other changes make use of kernel types as well as user-exportable ones
and fix format specifier issues.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.c | 20 ++++++++++----------
drivers/staging/android/binder.h | 8 ++++----
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index b7a8bec..06004cf 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1271,7 +1271,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
case BINDER_TYPE_WEAK_HANDLE: {
struct binder_ref *ref = binder_get_ref(proc, fp->handle);
if (ref == NULL) {
- pr_err("transaction release %d bad handle %ld\n",
+ pr_err("transaction release %d bad handle %d\n",
debug_id, fp->handle);
break;
}
@@ -1283,13 +1283,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
case BINDER_TYPE_FD:
binder_debug(BINDER_DEBUG_TRANSACTION,
- " fd %ld\n", fp->handle);
+ " fd %d\n", fp->handle);
if (failed_at)
task_close_fd(proc, fp->handle);
break;
default:
- pr_err("transaction release %d bad object type %lx\n",
+ pr_err("transaction release %d bad object type %x\n",
debug_id, fp->type);
break;
}
@@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_WEAK_HANDLE: {
struct binder_ref *ref = binder_get_ref(proc, fp->handle);
if (ref == NULL) {
- binder_user_error("%d:%d got transaction with invalid handle, %ld\n",
+ binder_user_error("%d:%d got transaction with invalid handle, %d\n",
proc->pid,
thread->pid, fp->handle);
return_error = BR_FAILED_REPLY;
@@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
if (!(in_reply_to->flags & TF_ACCEPT_FDS)) {
- binder_user_error("%d:%d got reply with fd, %ld, but target does not allow fds\n",
+ binder_user_error("%d:%d got reply with fd, %d, but target does not allow fds\n",
proc->pid, thread->pid, fp->handle);
return_error = BR_FAILED_REPLY;
goto err_fd_not_allowed;
}
} else if (!target_node->accept_fds) {
- binder_user_error("%d:%d got transaction with fd, %ld, but target does not allow fds\n",
+ binder_user_error("%d:%d got transaction with fd, %d, but target does not allow fds\n",
proc->pid, thread->pid, fp->handle);
return_error = BR_FAILED_REPLY;
goto err_fd_not_allowed;
@@ -1604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
file = fget(fp->handle);
if (file == NULL) {
- binder_user_error("%d:%d got transaction with invalid fd, %ld\n",
+ binder_user_error("%d:%d got transaction with invalid fd, %d\n",
proc->pid, thread->pid, fp->handle);
return_error = BR_FAILED_REPLY;
goto err_fget_failed;
@@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc *proc,
task_fd_install(target_proc, target_fd, file);
trace_binder_transaction_fd(t, fp->handle, target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
- " fd %ld -> %d\n", fp->handle, target_fd);
+ " fd %d -> %d\n", fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
default:
- binder_user_error("%d:%d got transaction with invalid object type, %lx\n",
+ binder_user_error("%d:%d got transaction with invalid object type, %x\n",
proc->pid, thread->pid, fp->type);
return_error = BR_FAILED_REPLY;
goto err_bad_object_type;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
goto err;
}
binder_debug(BINDER_DEBUG_READ_WRITE,
- "%d:%d write %zd at %08lx, read %zd at %08lx\n",
+ "%d:%d write %zd at %016lx, read %zd at %016lx\n",
proc->pid, thread->pid, bwr.write_size,
bwr.write_buffer, bwr.read_size, bwr.read_buffer);
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index edab249..2f94d16 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;
/* 8 bytes of data. */
union {
void __user *binder; /* local object */
- signed long handle; /* remote object */
+ __s32 handle; /* remote object */
};
/* extra data associated with local object */
@@ -78,7 +78,7 @@ struct binder_write_read {
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
};
/* This is the current protocol version. */
--
1.7.9.5
BinderDriverCommands mirror the ioctl usage. Thus the size of the
structure passed through the interface should be used to generate the
ioctl No.
The change reflects the type being passed from the user space-a pointer
to a binder_buffer. This change should not affect the existing 32bit
user space since BC_FREE_BUFFER is computed as:
#define _IOW(type,nr,size) \
((type) << _IOC_TYPESHIFT) | \
((nr) << _IOC_NRSHIFT) | \
((size) << _IOC_SIZESHIFT))
and for a 32bit compiler BC_FREE_BUFFER will have the same computed
value. This change will also ease our work in differentiating
BC_FREE_BUFFER from COMPAT_BC_FREE_BUFFER.
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
Acked-by: Arve Hjønnevåg <[email protected]>
---
drivers/staging/android/binder.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 1761541..c3562c4 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -265,7 +265,7 @@ enum binder_driver_command_protocol {
* Else you have acquired a primary reference on the object.
*/
- BC_FREE_BUFFER = _IOW('c', 3, int),
+ BC_FREE_BUFFER = _IOW('c', 3, void *),
/*
* void *: ptr to transaction data received on a read
*/
--
1.7.9.5
This change mirrors the userspace operation where struct binder_write_read
members that specify the buffer size and consumed size are size_t elements.
The patch also fixes the binder_thread_write() and binder_thread_read()
functions prototypes to conform with the definition of binder_write_read.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.c | 10 +++++-----
drivers/staging/android/binder.h | 8 ++++----
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..b7a8bec 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
}
int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, size_t size, size_t *consumed)
{
uint32_t cmd;
void __user *ptr = buffer + *consumed;
@@ -2080,8 +2080,8 @@ static int binder_has_thread_work(struct binder_thread *thread)
static int binder_thread_read(struct binder_proc *proc,
struct binder_thread *thread,
- void __user *buffer, int size,
- signed long *consumed, int non_block)
+ void __user *buffer, size_t size,
+ size_t *consumed, int non_block)
{
void __user *ptr = buffer + *consumed;
void __user *end = buffer + size;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
goto err;
}
binder_debug(BINDER_DEBUG_READ_WRITE,
- "%d:%d write %ld at %08lx, read %ld at %08lx\n",
+ "%d:%d write %zd at %08lx, read %zd at %08lx\n",
proc->pid, thread->pid, bwr.write_size,
bwr.write_buffer, bwr.read_size, bwr.read_buffer);
@@ -2604,7 +2604,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
}
}
binder_debug(BINDER_DEBUG_READ_WRITE,
- "%d:%d wrote %ld of %ld, read return %ld of %ld\n",
+ "%d:%d wrote %zd of %zd, read return %zd of %zd\n",
proc->pid, thread->pid, bwr.write_consumed, bwr.write_size,
bwr.read_consumed, bwr.read_size);
if (copy_to_user(ubuf, &bwr, sizeof(bwr))) {
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dbe81ce..edab249 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -67,11 +67,11 @@ struct flat_binder_object {
*/
struct binder_write_read {
- signed long write_size; /* bytes to write */
- signed long write_consumed; /* bytes consumed by driver */
+ size_t write_size; /* bytes to write */
+ size_t write_consumed; /* bytes consumed by driver */
unsigned long write_buffer;
- signed long read_size; /* bytes to read */
- signed long read_consumed; /* bytes consumed by driver */
+ size_t read_size; /* bytes to read */
+ size_t read_consumed; /* bytes consumed by driver */
unsigned long read_buffer;
};
--
1.7.9.5
The Android userspace aligns the data written to the binder buffers to
4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
Android userspace we can have a buffer looking like this:
platform buffer(binder_cmd pointer) size
32/32 32b 32b 8B
64/32 32b 64b 12B
64/64 32b 64b 12B
Thus the kernel needs to check that the buffer size is aligned to 4bytes
not to (void *) that will be 8bytes on 64bit machines.
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 06004cf..6436f22 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -658,8 +658,8 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
return NULL;
}
- size = ALIGN(data_size, sizeof(void *)) +
- ALIGN(offsets_size, sizeof(void *));
+ size = ALIGN(data_size, sizeof(u32)) +
+ ALIGN(offsets_size, sizeof(u32));
if (size < data_size || size < offsets_size) {
binder_user_error("%d: got transaction with invalid size %zd-%zd\n",
@@ -807,8 +807,8 @@ static void binder_free_buf(struct binder_proc *proc,
buffer_size = binder_buffer_size(proc, buffer);
- size = ALIGN(buffer->data_size, sizeof(void *)) +
- ALIGN(buffer->offsets_size, sizeof(void *));
+ size = ALIGN(buffer->data_size, sizeof(u32)) +
+ ALIGN(buffer->offsets_size, sizeof(u32));
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: binder_free_buf %p size %zd buffer_size %zd\n",
@@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
struct flat_binder_object *fp;
if (*offp > buffer->data_size - sizeof(*fp) ||
buffer->data_size < sizeof(*fp) ||
- !IS_ALIGNED(*offp, sizeof(void *))) {
+ !IS_ALIGNED(*offp, sizeof(u32))) {
pr_err("transaction release %d bad offset %zd, size %zd\n",
debug_id, *offp, buffer->data_size);
continue;
@@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
struct flat_binder_object *fp;
if (*offp > t->buffer->data_size - sizeof(*fp) ||
t->buffer->data_size < sizeof(*fp) ||
- !IS_ALIGNED(*offp, sizeof(void *))) {
+ !IS_ALIGNED(*offp, sizeof(u32))) {
binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
proc->pid, thread->pid, *offp);
return_error = BR_FAILED_REPLY;
--
1.7.9.5
Hi all,
Any feedback or comments on this patch set?
Thanks,
Serban
On 12/04/13 13:25, Serban Constantinescu wrote:
> Hi all,
>
> This set of patches will clean-up and fix some of the issues that arise
> with the current binder interface when moving to a 64bit kernel. All these
> changes will not affect the existing 32bit Android interface and are meant
> to stand as the base for the 64bit binder compat layer(kernel or userpsace).
>
> The patch set has been successfully tested with a 64bit Linux userspace and
> 64bit binder unit-tests.
>
> This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
> kernel binder compat layer.
>
> Changes for v3:
> 1: Dropped the patch that was replacing uint32_t types with unsigned int
> 2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's
> staging tree)
> 3: Split one patch into two: 'modify binder_write_read' and '64bit changes'
> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
> 5: Modified the binder command IOCTL declarations according to Arve's review
>
> Changes for v2:
> 1: 1/7: Modified the commit message according to Greg's feedback;
> 2: 3/7: Merged with the patch fixing the printk format specifiers.
>
> Serban Constantinescu (6):
> staging: android: binder: modify struct binder_write_read to use
> size_t
> staging: android: binder: fix binder interface for 64bit compat layer
> staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
> staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
> staging: android: binder: fix alignment issues
> staging: android: binder: replace types with portable ones
>
> drivers/staging/android/binder.c | 40 ++++++++++++++++-----------------
> drivers/staging/android/binder.h | 46 +++++++++++++++++++-------------------
> 2 files changed, 43 insertions(+), 43 deletions(-)
>
On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
<[email protected]> wrote:
> Hi all,
>
> Any feedback or comments on this patch set?
>
You don't seem to have addressed my feedback on the previous patch set.
--
Arve Hj?nnev?g
On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
> The patch set has been successfully tested with a 64bit Linux userspace and
> 64bit binder unit-tests.
>
> This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
> kernel binder compat layer.
I've tried to replace ugly hack with your patches to be able to boot
32-bit userspace on x86-64. It fails. :(
--
Kirill A. Shutemov
Hi Arve,
On 30/04/13 00:13, Arve Hjønnevåg wrote:
> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
> <[email protected]> wrote:
>> Hi all,
>>
>> Any feedback or comments on this patch set?
>>
>
> You don't seem to have addressed my feedback on the previous patch set.
For v3 I have modified the following according to your review:
> Changes for v3:
> 1: Dropped the patch that was replacing uint32_t types with unsigned int
> 2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's
> staging tree)
> 3: Split one patch into two: 'modify binder_write_read' and '64bit changes'
> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
> 5: Modified the binder command IOCTL declarations according to Arve's review
The following were left out:
> On 11/04/13 22:40, Arve Hjønnevåg wrote:
> OK, relaxing the alignment requirement for *offp to what the hardware
>> requires makes sense. Is there any macros in the kernel to help with
>> this, instead of hard-coding it to 4 bytes?
There is no kernel macro that I know which will help here(one that
springs to mind is PTR_ALIGN but it aligns to (unsigned long) - we need
one that aligns to (u32)). Any ideas?
> On 11/04/13 21:38, Arve Hjønnevåg wrote:
> OK, but if you are using this change let a 64 bit user-space know that
>> the driver has been fixed, then this patch needs to go after the
>> patches that change the structures on 64 bit systems.
For 32bit systems nothing has changed so they will continue to work as
before. For 64bit systems the size of binder_version was signed long
before the patch and __s32 after the patch is applied. Thus a 64bit
system using the old interface will fail immediately after opening the
binder driver, while cheeking the binder version (since the
BINDER_VERSION ioctl will be different pre/post patch - size of
binder_version differs).
For 64/32 systems once I will have the userspace wrapper ready I will
add another ioctl(as discussed) that will check if the driver is 64bit
ready(among the first things to do on binder_open).
Please let me know if there is anything that skipped my review and you
would like to integrate in this patch set.
Thanks for your feedback and help,
Serban
Hi Kirill,
On 30/04/13 08:31, Kirill A. Shutemov wrote:
> On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
>> The patch set has been successfully tested with a 64bit Linux userspace and
>> 64bit binder unit-tests.
>>
>> This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
>> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
>> kernel binder compat layer.
>
> I've tried to replace ugly hack with your patches to be able to boot
> 32-bit userspace on x86-64. It fails. :(
>
This patch set fixes the binder interface so that it works on 32/32 or
64/64 systems and preps the driver for the compat layer - 64bit
kernel/32bit userspace. As discussed with the Android developers the
binder compat will live in the userspace. Thus for a 64bit kernel the
calls coming from 32 or 64bit file-systems will look alike.
Thus for the moment it won't work.
Thanks,
Serban
On Tue, Apr 30, 2013 at 09:52:34AM +0100, Serban Constantinescu wrote:
> Hi Kirill,
>
> On 30/04/13 08:31, Kirill A. Shutemov wrote:
> >On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
> >>The patch set has been successfully tested with a 64bit Linux userspace and
> >>64bit binder unit-tests.
> >>
> >>This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
> >>and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
> >>kernel binder compat layer.
> >
> >I've tried to replace ugly hack with your patches to be able to boot
> >32-bit userspace on x86-64. It fails. :(
> >
>
> This patch set fixes the binder interface so that it works on 32/32
> or 64/64 systems and preps the driver for the compat layer - 64bit
> kernel/32bit userspace. As discussed with the Android developers the
> binder compat will live in the userspace.
Does the compat layer already exist or it's a plan?
--
Kirill A. Shutemov
On 30/04/13 11:04, Kirill A. Shutemov wrote:
> On Tue, Apr 30, 2013 at 09:52:34AM +0100, Serban Constantinescu wrote:
>> Hi Kirill,
>>
>> On 30/04/13 08:31, Kirill A. Shutemov wrote:
>>> On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
>>>> The patch set has been successfully tested with a 64bit Linux userspace and
>>>> 64bit binder unit-tests.
>>>>
>>>> This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
>>>> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
>>>> kernel binder compat layer.
>>>
>>> I've tried to replace ugly hack with your patches to be able to boot
>>> 32-bit userspace on x86-64. It fails. :(
>>>
>>
>> This patch set fixes the binder interface so that it works on 32/32
>> or 64/64 systems and preps the driver for the compat layer - 64bit
>> kernel/32bit userspace. As discussed with the Android developers the
>> binder compat will live in the userspace.
>
> Does the compat layer already exist or it's a plan?
We were working with a kernel compat layer. However this wasn't
encouraged by the Android developers who would prefer a userspace binder
compat.
The changes in this patch set fix the kernel interface so that it will
work with the yet to be developed userspace wrapper.
Some support for 64/32 systems has already been integrated - see ashmem.
Thanks,
Serban
On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
<[email protected]> wrote:
> Hi Arve,
>
>
> On 30/04/13 00:13, Arve Hj?nnev?g wrote:
>>
>> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
>> <[email protected]> wrote:
>>>
>>> Hi all,
>>>
>>> Any feedback or comments on this patch set?
>>>
>>
>> You don't seem to have addressed my feedback on the previous patch set.
>
>
> For v3 I have modified the following according to your review:
>
>
>> Changes for v3:
>> 1: Dropped the patch that was replacing uint32_t types with unsigned int
>> 2: Dropped the patch fixing the IOCTL types(since it has been added to
>> Greg's
>> staging tree)
>> 3: Split one patch into two: 'modify binder_write_read' and '64bit
>> changes'
>> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>> review
>> 5: Modified the binder command IOCTL declarations according to Arve's
>> review
>
>
> The following were left out:
>
>> On 11/04/13 22:40, Arve Hj?nnev?g wrote:
>> OK, relaxing the alignment requirement for *offp to what the hardware
>>>
>>> requires makes sense. Is there any macros in the kernel to help with
>>> this, instead of hard-coding it to 4 bytes?
>
>
> There is no kernel macro that I know which will help here(one that springs
> to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that
> aligns to (u32)). Any ideas?
>
Perhaps using __alignof__(struct flat_binder_object) will work. This
is the least important part of that change however. I saw no response
to my concern that your changes can cause less memory to be allocated
than you write to.
>> On 11/04/13 21:38, Arve Hj?nnev?g wrote:
>> OK, but if you are using this change let a 64 bit user-space know that
>>>
>>> the driver has been fixed, then this patch needs to go after the
>>> patches that change the structures on 64 bit systems.
>
>
> For 32bit systems nothing has changed so they will continue to work as
> before. For 64bit systems the size of binder_version was signed long before
> the patch and __s32 after the patch is applied. Thus a 64bit system using
> the old interface will fail immediately after opening the binder driver,
> while cheeking the binder version (since the BINDER_VERSION ioctl will be
> different pre/post patch - size of binder_version differs).
>
> For 64/32 systems once I will have the userspace wrapper ready I will add
> another ioctl(as discussed) that will check if the driver is 64bit
> ready(among the first things to do on binder_open).
>
Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
the driver is usable on a 64 bit system?
> Please let me know if there is anything that skipped my review and you would
> like to integrate in this patch set.
>
It may be better to reply to my original emails instead of copying
bits of them here.
> Thanks for your feedback and help,
> Serban
>
--
Arve Hj?nnev?g
On 01/05/13 00:52, Arve Hjønnevåg wrote:
> On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
> <[email protected]> wrote:
>> Hi Arve,
>>
>>
>> On 30/04/13 00:13, Arve Hjønnevåg wrote:
>>>
>>> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
>>> <[email protected]> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Any feedback or comments on this patch set?
>>>>
>>>
>>> You don't seem to have addressed my feedback on the previous patch set.
>>
>>
>> For v3 I have modified the following according to your review:
>>
>>
>>> Changes for v3:
>>> 1: Dropped the patch that was replacing uint32_t types with unsigned int
>>> 2: Dropped the patch fixing the IOCTL types(since it has been added to
>>> Greg's
>>> staging tree)
>>> 3: Split one patch into two: 'modify binder_write_read' and '64bit
>>> changes'
>>> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>>> review
>>> 5: Modified the binder command IOCTL declarations according to Arve's
>>> review
>>
>>
>> The following were left out:
>>
>>> On 11/04/13 22:40, Arve Hjønnevåg wrote:
>>> OK, relaxing the alignment requirement for *offp to what the hardware
>>>>
>>>> requires makes sense. Is there any macros in the kernel to help with
>>>> this, instead of hard-coding it to 4 bytes?
>>
>>
>> There is no kernel macro that I know which will help here(one that springs
>> to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that
>> aligns to (u32)). Any ideas?
>>
>
> Perhaps using __alignof__(struct flat_binder_object) will work. This
> is the least important part of that change however. I saw no response
> to my concern that your changes can cause less memory to be allocated
> than you write to.
This can happen for situations where (buffer_start + buffer_size) are
not aligned to (void *), because offset_start is calculated as:
> offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
Thus you can have a situation where instead of reading offset[i] you
will read (offset[i] >> 32 | offset[i+1] << 32) (offset is size_t -
8byte for 64bit systems).
I will address this issue in v4 of the patch set.
>
>>> On 11/04/13 21:38, Arve Hjønnevåg wrote:
>>> OK, but if you are using this change let a 64 bit user-space know that
>>>>
>>>> the driver has been fixed, then this patch needs to go after the
>>>> patches that change the structures on 64 bit systems.
>>
>>
>> For 32bit systems nothing has changed so they will continue to work as
>> before. For 64bit systems the size of binder_version was signed long before
>> the patch and __s32 after the patch is applied. Thus a 64bit system using
>> the old interface will fail immediately after opening the binder driver,
>> while cheeking the binder version (since the BINDER_VERSION ioctl will be
>> different pre/post patch - size of binder_version differs).
>>
>> For 64/32 systems once I will have the userspace wrapper ready I will add
>> another ioctl(as discussed) that will check if the driver is 64bit
>> ready(among the first things to do on binder_open).
>>
>
> Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
> the driver is usable on a 64 bit system?
Leaving the binder_version as a long will cause the BINDER_VERSION ioctl
to fail just on 64/32 - since the size will be different between 32bit
compilers and 64bit compilers. The call will succeed on 64/64 and 32/32
(since they use the same kernel headers).
>> Please let me know if there is anything that skipped my review and you would
>> like to integrate in this patch set.
>>
>
> It may be better to reply to my original emails instead of copying
> bits of them here.
I will do that! I did not understand your initial reply to the buffer
size issue, my fault!
Thanks for your feedback,
Serban
--
Best Regards,
Serban Constantinescu
PDSW Engineer ARM Ltd.