| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <678F3D1BB717D949B966B68EAEB446ED24E31270@dggemm526-mbx.china.huawei.com>
Date: Fri, 4 Jan 2019 01:42:51 +0000
From: "Zengtao (B)" <prime.zeng@...ilicon.com>
To: Laura Abbott <labbott@...hat.com>,
"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>,
Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] staging: android: ion: add buffer flag update ioctl
>-----Original Message-----
>From: Laura Abbott [mailto:labbott@...hat.com]
>Sent: Thursday, January 03, 2019 6:31 AM
>To: Zengtao (B) <prime.zeng@...ilicon.com>; sumit.semwal@...aro.org
>Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Arve Hjønnevåg
><arve@...roid.com>; Todd Kjos <tkjos@...roid.com>; Martijn Coenen
><maco@...roid.com>; Joel Fernandes <joel@...lfernandes.org>;
>devel@...verdev.osuosl.org; dri-devel@...ts.freedesktop.org;
>linaro-mm-sig@...ts.linaro.org; linux-kernel@...r.kernel.org
>Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl
>
>On 12/23/18 6:47 PM, Zengtao (B) wrote:
>> Hi laura:
>>
>>> -----Original Message-----
>>> From: Laura Abbott [mailto:labbott@...hat.com]
>>> Sent: Friday, December 21, 2018 4:50 AM
>>> To: Zengtao (B) <prime.zeng@...ilicon.com>;
>sumit.semwal@...aro.org
>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Arve
>Hjønnevåg
>>> <arve@...roid.com>; Todd Kjos <tkjos@...roid.com>; Martijn
>Coenen
>>> <maco@...roid.com>; Joel Fernandes <joel@...lfernandes.org>;
>>> devel@...verdev.osuosl.org; dri-devel@...ts.freedesktop.org;
>>> linaro-mm-sig@...ts.linaro.org; linux-kernel@...r.kernel.org
>>> Subject: Re: [PATCH] staging: android: ion: add buffer flag update
>>> ioctl
>>>
>>> On 12/19/18 5:39 PM, Zengtao (B) wrote:
>>>> Hi laura:
>>>>
>>>>> -----Original Message-----
>>>>> From: Laura Abbott [mailto:labbott@...hat.com]
>>>>> Sent: Thursday, December 20, 2018 2:10 AM
>>>>> To: Zengtao (B) <prime.zeng@...ilicon.com>;
>>> sumit.semwal@...aro.org
>>>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Arve
>>> Hjønnevåg
>>>>> <arve@...roid.com>; Todd Kjos <tkjos@...roid.com>; Martijn
>>> Coenen
>>>>> <maco@...roid.com>; Joel Fernandes <joel@...lfernandes.org>;
>>>>> devel@...verdev.osuosl.org; dri-devel@...ts.freedesktop.org;
>>>>> linaro-mm-sig@...ts.linaro.org; linux-kernel@...r.kernel.org
>>>>> Subject: Re: [PATCH] staging: android: ion: add buffer flag update
>>>>> ioctl
>>>>>
>>>>> On 12/19/18 9:19 AM, Zeng Tao wrote:
>>>>>> In some usecases, the buffer cached attribute is not determined at
>>>>>> allocation time, it's determined just before the real cpu mapping.
>>>>>> And from the memory view of point, a buffer should not have the
>>>>> cached
>>>>>> attribute util is really mapped by the cpu. So in this patch, we
>>>>>> introduced the new ioctl command to target the requirement.
>>>>>>
>>>>>
>>>>> This is racy and error prone. Can you explain more what problem
>you
>>>>> are trying to solve?
>>>>
>>>> My use case is like this:
>>>> 1. There are two process A and B, A takes case of ion buffer
>>>> allocation,
>>> and
>>>> pass the buffer fd to B, then B maps and uses it.
>>>> 2. Process B need to map the buffer with different cached attribute
>>>> for different use case, for example, if the buffer is used for pure
>>>> software algorithm, then we need to map it as cached, otherwise
>>>> non-cached, and B needs to deal with both cases.
>>>> And unfortunately the mmap syscall takes no cached flags and we
>>>> can't decide the cache attribute when we are doing the mmap, so I
>>>> introduce new the ioctl even though I think the solution is not as
>good.
>>>>
>>>>
>>>
>>> Thanks for the explanation, this was about the use case I expected.
>>> I'm pretty sure I had this exact problem once upon a time and we
>>> didn't come up with a solution. I'd still like to get rid of uncached
>>> buffers in general and just use cached buffers (see
>>> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/201
>>> 8-N
>>> ovember/128842.html)
>>> What's your usecase for uncached buffers?
>>
>> Some buffers are only used by HW, in this case, we tend to use
>uncached buffers.
>> But sometimes the SW need to read few buffer contents for debug
>> purpose and no synchronization is needed.
>>
>
>If this is primarily for debug, that's not really a compelling reason to
>support uncached buffers. It's incredibly difficult to do uncached buffers
>correctly I'd rather spend effort on making the cached use cases work
>better.
>
No, not only for debug, I meant if the buffers are uncached, the SW will only mmap
them for debug or even don't need to mmap them.
So for the two kinds of buffers:
1. uncached: only the HW need to access it, the SW don't need to mmap it or just for debug.
2. cached: both the HW and the SW need to access it.
The ION is designed as a generalized memory manager, I think we should cover as many
requirements as we can in the ION design, so I 'd rather that we keep the uncached support.
And I don’t quite understand the difficulty you mentioned to support the uncached buffers, would
you explain a little more about it.
Thanks
Zengtao
>
>>>
>>>>>
>>>>>> Signed-off-by: Zeng Tao <prime.zeng@...ilicon.com>
>>>>>> ---
>>>>>> drivers/staging/android/ion/ion-ioctl.c | 4 ++++
>>>>>> drivers/staging/android/ion/ion.c | 17
>>> +++++++++++++++++
>>>>>> drivers/staging/android/ion/ion.h | 1 +
>>>>>> drivers/staging/android/uapi/ion.h | 22
>>>>> ++++++++++++++++++++++
>>>>>> 4 files changed, 44 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>>>>> b/drivers/staging/android/ion/ion-ioctl.c
>>>>>> index a8d3cc4..60bb702 100644
>>>>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>>>>> @@ -12,6 +12,7 @@
>>>>>>
>>>>>> union ion_ioctl_arg {
>>>>>> struct ion_allocation_data allocation;
>>>>>> + struct ion_buffer_flag_data update;
>>>>>> struct ion_heap_query query;
>>>>>> };
>>>>>>
>>>>>> @@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int
>>>>>> cmd, unsigned long arg)
>>>>>>
>>>>>> break;
>>>>>> }
>>>>>> + case ION_IOC_BUFFER_UPDATE:
>>>>>> + ret = ion_buffer_update(data.update.fd, data.update.flags);
>>>>>> + break;
>>>>>> case ION_IOC_HEAP_QUERY:
>>>>>> ret = ion_query_heaps(&data.query);
>>>>>> break;
>>>>>> diff --git a/drivers/staging/android/ion/ion.c
>>>>>> b/drivers/staging/android/ion/ion.c
>>>>>> index 9907332..f1404dc 100644
>>>>>> --- a/drivers/staging/android/ion/ion.c
>>>>>> +++ b/drivers/staging/android/ion/ion.c
>>>>>> @@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int
>>>>> heap_id_mask, unsigned int flags)
>>>>>> return fd;
>>>>>> }
>>>>>>
>>>>>> +int ion_buffer_update(unsigned int fd, unsigned int flags) {
>>>>>> + struct dma_buf *dmabuf;
>>>>>> + struct ion_buffer *buffer;
>>>>>> +
>>>>>> + dmabuf = dma_buf_get(fd);
>>>>>> +
>>>>>> + if (!dmabuf)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + buffer = dmabuf->priv;
>>>>>> + buffer->flags = flags;
>>>>>> + dma_buf_put(dmabuf);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> int ion_query_heaps(struct ion_heap_query *query)
>>>>>> {
>>>>>> struct ion_device *dev = internal_dev; diff --git
>>>>>> a/drivers/staging/android/ion/ion.h
>>>>>> b/drivers/staging/android/ion/ion.h
>>>>>> index c006fc1..99bf9ab 100644
>>>>>> --- a/drivers/staging/android/ion/ion.h
>>>>>> +++ b/drivers/staging/android/ion/ion.h
>>>>>> @@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page
>*page,
>>>>> size_t size, pgprot_t pgprot);
>>>>>> int ion_alloc(size_t len,
>>>>>> unsigned int heap_id_mask,
>>>>>> unsigned int flags);
>>>>>> +int ion_buffer_update(unsigned int fd, unsigned int flags);
>>>>>>
>>>>>> /**
>>>>>> * ion_heap_init_shrinker
>>>>>> diff --git a/drivers/staging/android/uapi/ion.h
>>>>>> b/drivers/staging/android/uapi/ion.h
>>>>>> index 5d70098..99753fc 100644
>>>>>> --- a/drivers/staging/android/uapi/ion.h
>>>>>> +++ b/drivers/staging/android/uapi/ion.h
>>>>>> @@ -74,6 +74,20 @@ struct ion_allocation_data {
>>>>>> __u32 unused;
>>>>>> };
>>>>>>
>>>>>> +/**
>>>>>> + * struct ion_buffer_flag_data - metadata passed from userspace
>>>>>> +for update
>>>>>> + * buffer flags
>>>>>> + * @fd: file descriptor of the buffer
>>>>>> + * @flags: flags passed to the buffer
>>>>>> + *
>>>>>> + * Provided by userspace as an argument to the ioctl */
>>>>>> +
>>>>>> +struct ion_buffer_flag_data {
>>>>>> + __u32 fd;
>>>>>> + __u32 flags;
>>>>>> +}
>>>>>> +
>>>>>> #define MAX_HEAP_NAME 32
>>>>>>
>>>>>> /**
>>>>>> @@ -116,6 +130,14 @@ struct ion_heap_query {
>>>>>> struct ion_allocation_data)
>>>>>>
>>>>>> /**
>>>>>> + * DOC: ION_IOC_BUFFER_UPDATE - update the specified ion
>buffer
>>>>> flags
>>>>>> + *
>>>>>> + * Takes an ion_buffer_flag_data structure and returns the result
>>>>>> +of the
>>>>>> + * buffer flag update operation.
>>>>>> + */
>>>>>> +#define ION_IOC_BUFFER_UPDATE _IOWR(ION_IOC_MAGIC, 1, \
>>>>>> + struct ion_buffer_flag_data)
>>>>>> +/**
>>>>>> * DOC: ION_IOC_HEAP_QUERY - information about available
>>> heaps
>>>>>> *
>>>>>> * Takes an ion_heap_query structure and populates
>information
>>>>> about
>>>>>>
>>>>
>>
Powered by blists - more mailing lists