On Thu, Apr 11, 2013 at 12:02 PM, Serban Constantinescu <serban.constantine...@arm.com> wrote: > On 10/04/13 23:30, Arve Hjønnevåg wrote: >> >> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu >> <serban.constantine...@arm.com> wrote: >>> >>> On 10/04/13 00:58, Arve Hjønnevåg wrote: >>>> >>>> >>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu >>>> <serban.constantine...@arm.com> wrote: >>>>> >>>>> >>>>> 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. >>>>> >>>> >>>> Do we not want the pointers to be 8 byte aligned on 64bit platforms? >>> >>> >>> >>> No since here we do not align pointers we align binder_buffers and >>> offsets >>> in a buffer. >>> >> >> Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we >> should keep the start address of the struct 8 byte aligned as well. > > > Most of 64bit compilers will try to align pointers within a structure to > natural boundaries. However all 64bit variants of currently supported > Android architectures can service unaligned accesses(possibly with a > performance degradation compared to an aligned access). > > You can take a look at alignment requirements for AArch64 here > http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf > chapter 4. > > What we are modifying in this patch is the alignment requirements on the > buffer size(as seen above - arbitrary size 4byte aligned) and the alignment > check on offp. >
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? I don't think there is any reason to not keep the binder_buffer and offsets buffer that the kernel allocates aligned to 8 bytes on a 64 bit system. Also, I don't see any changes to where the offsets buffer starts in this patch, so if datasize is not 8 bytes aligned you seem to allocate less memory than you use. > Let's take a look at what offp does. The userspace will write object > references to a buffer using: > >>> 820 status_t Parcel::writeObject(const flat_binder_object& val, bool >>> nullMetaData) >>> ... >>> 826 *reinterpret_cast<flat_binder_object*>(mData+mDataPos) = >>> val; > > > Buffer > |---------------------------------------|val > | | > |->mData |->mDataPos > > where mData is the start of the buffer and mDataPos the current position > within the buffer(equivalent to offp in the kernel space). Since the buffer > is guaranteed to be u32 aligned but not u64 aligned the pointer to > flat_binder_object might live on a unaligned boundary(offp will always be > aligned to sizeof(u32) - see Parcel::writeAligned()). > > However this will happen only on buffers where at the time we write the > next object reference(val) the buffer cursor(mDataPos) happens not to be on > a multiple of sizeof(void *). > > Adding an alignment check in the userspace might be more costly than > servicing the unaligned access(for AArch64 serviced in hardware). Also we > will save some memory by not adding the padding. > > On the other hand if instead of writing a pointer we write a 64bit mutex > lock to an unaligned address and than try to read it in the kernel side > things are not guaranteed to be sane. The compiler could make the assumption > that the lock is natural aligned and use load/store exclusive that will fail > on an unaligned address. However for this situation we can extend the > userspace API and add a mutex write primitive like: > > >> status_t Parcel::writeMutex(mutex lock) >> ... >> *reinterpret_cast<mutex>(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock; > > > I am not aware of any situation where you will have 64bit mutexes passed in > a binder buffer but you would probably know more about this. Since all > writes to the buffer are 32bit aligned a 32bit mutex would not suffer any > alignment issues. > > Let me know what are your thoughts about this. > Storing a mutex in a parcel does not make sense. The data in the parcel is a copy of the data passed in, and the parcel seen by the remote process is a copy of the parcel that sender created. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
