Re: [Linaro-mm-sig] [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions: - since alignment management has been remove from ion-core, should it be also removed from ioctl structure ? - can you we ride off ion_handle (at least in userland) and only export a dma-buf descriptor ?
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Benjamin
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Follow Linaro: Facebook | Twitter | Blog
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
Benjamin
Thanks, Laura
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
-Brian
On 10/03/17 10:31, Brian Starkey wrote:
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
For that use-case, though, alignment alone doesn't necessarily help - you need the whole allocation granularity to match your block size (i.e. given a 1MB block size, asking for 17KB and getting back 17KB starting at a 1MB boundary doesn't help much - that whole 1MB needs to be allocated and everyone needs to know it to ensure that the whole lot can be mapped safely). Now, whether it's down to the callers or the heap implementations to decide and enforce that granularity is another question, but provided allocations are at least naturally aligned to whatever the granularity is (which is a reasonable assumption to bake in) then it's all good.
Robin.
-Brian
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Mar 10, 2017 at 11:46:42AM +0000, Robin Murphy wrote:
On 10/03/17 10:31, Brian Starkey wrote:
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
For that use-case, though, alignment alone doesn't necessarily help - you need the whole allocation granularity to match your block size (i.e. given a 1MB block size, asking for 17KB and getting back 17KB starting at a 1MB boundary doesn't help much - that whole 1MB needs to be allocated and everyone needs to know it to ensure that the whole lot can be mapped safely). Now, whether it's down to the callers or the heap implementations to decide and enforce that granularity is another question, but provided allocations are at least naturally aligned to whatever the granularity is (which is a reasonable assumption to bake in) then it's all good.
Robin.
Agreed, alignment alone isn't enough. But lets assume that an app knows what a "good" granularity is, and always asks for allocation sizes which are suitably rounded to allow blocks to be used. Currently it looks like a "standard" ION_HEAP_TYPE_CARVEOUT heap would give me back just a PAGE_SIZE aligned buffer. So even *if* the caller knows its desired block size, there's no way for it to get guaranteed better alignment, which wouldn't be a bad feature to have.
Anyway as Daniel and Rob say, if the interface is designed properly this kind of extension would be possible later, or you can have a special heap with a larger granule.
I suppose it makes sense to remove it while there's no-one actually implementing it, in case an alternate method proves more usable.
-Brian
-Brian
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 03/10/2017 06:27 AM, Brian Starkey wrote:
On Fri, Mar 10, 2017 at 11:46:42AM +0000, Robin Murphy wrote:
On 10/03/17 10:31, Brian Starkey wrote:
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
For that use-case, though, alignment alone doesn't necessarily help - you need the whole allocation granularity to match your block size (i.e. given a 1MB block size, asking for 17KB and getting back 17KB starting at a 1MB boundary doesn't help much - that whole 1MB needs to be allocated and everyone needs to know it to ensure that the whole lot can be mapped safely). Now, whether it's down to the callers or the heap implementations to decide and enforce that granularity is another question, but provided allocations are at least naturally aligned to whatever the granularity is (which is a reasonable assumption to bake in) then it's all good.
Robin.
Agreed, alignment alone isn't enough. But lets assume that an app knows what a "good" granularity is, and always asks for allocation sizes which are suitably rounded to allow blocks to be used. Currently it looks like a "standard" ION_HEAP_TYPE_CARVEOUT heap would give me back just a PAGE_SIZE aligned buffer. So even *if* the caller knows its desired block size, there's no way for it to get guaranteed better alignment, which wouldn't be a bad feature to have.
Anyway as Daniel and Rob say, if the interface is designed properly this kind of extension would be possible later, or you can have a special heap with a larger granule.
I suppose it makes sense to remove it while there's no-one actually implementing it, in case an alternate method proves more usable.
-Brian
Part of the reason I want to remove it is to avoid confusion over callers thinking it will do anything on most heaps. I agree being able to specify a larger granularity would be beneficial but I don't think a dedicated field in the ABI is the right approach.
Thanks, Laura
On Fri, Mar 10, 2017 at 10:31:13AM +0000, Brian Starkey wrote:
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
Extending ioctl uapi is easy, trying to get rid of bad uapi is much harder. Given that right now we don't have an ion allocator that does alignment I think removing it makes sense. And if we go with lots of heaps, we might as well have an ion heap per alignment that your hw needs, so there's different ways to implement this in the future.
At least from the unix device memory allocator pov it's probably simpler to encode stuff like this into the heap name, instead of having to pass heap + list of additional properties/constraints. -Daniel
On Fri, Mar 10, 2017 at 7:40 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Mar 10, 2017 at 10:31:13AM +0000, Brian Starkey wrote:
Hi,
On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
[snip]
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
Is the only motivation for removing the alignment parameter that no-one got around to using it for something useful yet? The original comment was true - different devices do have different alignment requirements.
Better alignment can help SMMUs use larger blocks when mapping, reducing TLB pressure and the chance of a page table walk causing display underruns.
Extending ioctl uapi is easy, trying to get rid of bad uapi is much harder. Given that right now we don't have an ion allocator that does alignment I think removing it makes sense. And if we go with lots of heaps, we might as well have an ion heap per alignment that your hw needs, so there's different ways to implement this in the future.
slight correction: if you plan ahead (and do things like zero init if userspace passes in a smaller ioctl struct like drm_ioctl does), extending ioctl uapi is easy.. might be something worth fixing from the get-go..
BR, -R
At least from the unix device memory allocator pov it's probably simpler to encode stuff like this into the heap name, instead of having to pass heap + list of additional properties/constraints.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
Benjamin
Thanks, Laura
On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard benjamin.gaignard@linaro.org wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
I think ion should expose any heap that's also directly accessible to devices using dma_alloc(_coherent). That should leave very few things left, like your SMA heap.
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
On 03/12/2017 12:05 PM, Daniel Vetter wrote:
On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard benjamin.gaignard@linaro.org wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
> No one gave a thing about android in upstream, so Greg KH just dumped it > all into staging/android/. We've discussed ION a bunch of times, recorded > anything we'd like to fix in staging/android/TODO, and Laura's patch > series here addresses a big chunk of that.
> This is pretty much the same approach we (gpu folks) used to de-stage the > syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
I think ion should expose any heap that's also directly accessible to devices using dma_alloc(_coherent). That should leave very few things left, like your SMA heap.
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
I thought about that. One advantage with separate /dev/ion_$heap is that we don't have to worry about a limit of 32 possible heaps per system (32-bit heap id allocation field). But dealing with an ioctl seems easier than names. Userspace might be less likely to hardcode random id numbers vs. names as well.
Thanks, Laura
On Mon, Mar 13, 2017 at 5:09 PM, Laura Abbott labbott@redhat.com wrote:
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
I thought about that. One advantage with separate /dev/ion_$heap is that we don't have to worry about a limit of 32 possible heaps per system (32-bit heap id allocation field). But dealing with an ioctl seems easier than names. Userspace might be less likely to hardcode random id numbers vs. names as well.
other advantage, I think, is selinux (brought up elsewhere on this thread).. heaps at known fixed PAs are useful for certain sorts of attacks so being able to restrict access more easily seems like a good thing
BR, -R
On 03/13/2017 02:29 PM, Rob Clark wrote:
On Mon, Mar 13, 2017 at 5:09 PM, Laura Abbott labbott@redhat.com wrote:
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
I thought about that. One advantage with separate /dev/ion_$heap is that we don't have to worry about a limit of 32 possible heaps per system (32-bit heap id allocation field). But dealing with an ioctl seems easier than names. Userspace might be less likely to hardcode random id numbers vs. names as well.
other advantage, I think, is selinux (brought up elsewhere on this thread).. heaps at known fixed PAs are useful for certain sorts of attacks so being able to restrict access more easily seems like a good thing
BR, -R
Some other kind of filtering (BPF/LSM/???) might work as well (http://kernsec.org/files/lss2015/vanderstoep.pdf ?)
The fixed PA issue is a larger problem. We're never going to be able to get away from "this heap must exist at address X" problems but the location of CMA in general should be randomized. I haven't actually come up with a good proposal to this though.
I'd like for Ion to be a framework for memory allocation and not security exploits. Hopefully this isn't a pipe dream.
Thanks, Laura
2017-03-13 22:09 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/12/2017 12:05 PM, Daniel Vetter wrote:
On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard benjamin.gaignard@linaro.org wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote: > On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote: > >> No one gave a thing about android in upstream, so Greg KH just dumped it >> all into staging/android/. We've discussed ION a bunch of times, recorded >> anything we'd like to fix in staging/android/TODO, and Laura's patch >> series here addresses a big chunk of that. > >> This is pretty much the same approach we (gpu folks) used to de-stage the >> syncpt stuff. > > Well, there's also the fact that quite a few people have issues with the > design (like Laurent). It seems like a lot of them have either got more > comfortable with it over time, or at least not managed to come up with > any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
I think ion should expose any heap that's also directly accessible to devices using dma_alloc(_coherent). That should leave very few things left, like your SMA heap.
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
I thought about that. One advantage with separate /dev/ion_$heap
Should we use /devi/ion/$heap instead of /dev/ion_$heap ? I think it would be easier for user to look into one directory rather then in whole /dev to find the heaps
is that we don't have to worry about a limit of 32 possible heaps per system (32-bit heap id allocation field). But dealing with an ioctl seems easier than names. Userspace might be less likely to hardcode random id numbers vs. names as well.
In the futur I think that heap type will be replaced by a "get caps" ioctl which will describe heap capabilities. At least that is my understanding of kernel part of "unix memory allocator" project
Thanks, Laura
On 03/14/2017 07:47 AM, Benjamin Gaignard wrote:
2017-03-13 22:09 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/12/2017 12:05 PM, Daniel Vetter wrote:
On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard benjamin.gaignard@linaro.org wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch: > On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote: >> On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote: >> >>> No one gave a thing about android in upstream, so Greg KH just dumped it >>> all into staging/android/. We've discussed ION a bunch of times, recorded >>> anything we'd like to fix in staging/android/TODO, and Laura's patch >>> series here addresses a big chunk of that. >> >>> This is pretty much the same approach we (gpu folks) used to de-stage the >>> syncpt stuff. >> >> Well, there's also the fact that quite a few people have issues with the >> design (like Laurent). It seems like a lot of them have either got more >> comfortable with it over time, or at least not managed to come up with >> any better ideas in the meantime. > > See the TODO, it has everything a really big group (look at the patch for > the full Cc: list) figured needs to be improved at LPC 2015. We don't just > merge stuff because merging stuff is fun :-) > > Laurent was even in that group ... > -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
I think ion should expose any heap that's also directly accessible to devices using dma_alloc(_coherent). That should leave very few things left, like your SMA heap.
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel
I thought about that. One advantage with separate /dev/ion_$heap
Should we use /devi/ion/$heap instead of /dev/ion_$heap ? I think it would be easier for user to look into one directory rather then in whole /dev to find the heaps
If we decide to move away from /dev/ion we can consider it.
is that we don't have to worry about a limit of 32 possible heaps per system (32-bit heap id allocation field). But dealing with an ioctl seems easier than names. Userspace might be less likely to hardcode random id numbers vs. names as well.
In the futur I think that heap type will be replaced by a "get caps" ioctl which will describe heap capabilities. At least that is my understanding of kernel part of "unix memory allocator" project
I don't think it will be completely replaced. A heap can have multiple capabilities so I suspect there will need to be some cap -> allocation instance translation. Of course all this is wild speculation since much of the unix memory allocator isn't well defined yet.
Thanks, Laura
On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that.
This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
Are you concerned about actual heaps (e.g. a carveout at 0x80000000 vs a carveout at 0x60000000) or heap types?
For heap types, I think the policy can be strict - if it's generally useful then it should live in-tree in ion. Otherwise, it would be out-of-tree. I'd expect most "custom" heaps to be parameterisable to the point of being generally useful.
For actual heap instances, I would expect them to be communicated via reserved-memory regions or something similar, and so the maintenance burden is pretty low.
The existing query ioctl can allow heap IDs to get assigned dynamically at runtime, so there's no need to reserve "bit 6" for "CUSTOM_ACME_HEAP_1"
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
I might be thinking of a different type of "secure", but...
Should the security of secure heaps be enforced by OS-level permissions? I don't know about other architectures, but at least on arm/arm64 this is enforced in hardware; it doesn't matter who has access to the ion heap, because only secure devices (or the CPU running a secure process) is physically able to access the memory backing the buffer.
In fact, in the use-cases I know of, the process asking for the ion allocation is not a secure process, and so we wouldn't *want* to restrict the secure heap to be allocated from only by secure processes.
-Brian
Benjamin
Thanks, Laura
On 03/13/2017 03:54 AM, Brian Starkey wrote:
On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:
2017-03-09 18:38 GMT+01:00 Laura Abbott labbott@redhat.com:
On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
2017-03-06 17:04 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
> No one gave a thing about android in upstream, so Greg KH just dumped it > all into staging/android/. We've discussed ION a bunch of times, recorded > anything we'd like to fix in staging/android/TODO, and Laura's patch > series here addresses a big chunk of that.
> This is pretty much the same approach we (gpu folks) used to de-stage the > syncpt stuff.
Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime.
See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-)
Laurent was even in that group ... -Daniel
For me those patches are going in the right direction.
I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?
Yes, I think I'm going to go with the suggestion to fixup the ABI so we don't need the compat layer and as part of that I'm also dropping the align argument.
- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?
Yes, I think this is the right direction given we're breaking everything anyway. I was debating trying to keep the two but moving to only dma bufs is probably cleaner. The only reason I could see for keeping the handles is running out of file descriptors for dma-bufs but that seems unlikely.
In the future how can we add new heaps ? Some platforms have very specific memory allocation requirements (just have a look in the number of gem custom allocator in drm) Do you plan to add heap type/mask for each ?
Yes, that was my thinking.
My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain....
Are you concerned about actual heaps (e.g. a carveout at 0x80000000 vs a carveout at 0x60000000) or heap types?
For heap types, I think the policy can be strict - if it's generally useful then it should live in-tree in ion. Otherwise, it would be out-of-tree. I'd expect most "custom" heaps to be parameterisable to the point of being generally useful.
I'm willing to be reasonably permissive in what lives in tree. A good example would be something like a heap for the OMAP tiler which had weird hardware requirements. The associated devices that go with the heap should be well supported upstream though.
For actual heap instances, I would expect them to be communicated via reserved-memory regions or something similar, and so the maintenance burden is pretty low.
Yes. After the next round of review for this series I'm going to start thinking about properties for chunk and carveout heaps if nobody proposes something first.
The existing query ioctl can allow heap IDs to get assigned dynamically at runtime, so there's no need to reserve "bit 6" for "CUSTOM_ACME_HEAP_1"
Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution.
I might be thinking of a different type of "secure", but...
Should the security of secure heaps be enforced by OS-level permissions? I don't know about other architectures, but at least on arm/arm64 this is enforced in hardware; it doesn't matter who has access to the ion heap, because only secure devices (or the CPU running a secure process) is physically able to access the memory backing the buffer.
In fact, in the use-cases I know of, the process asking for the ion allocation is not a secure process, and so we wouldn't *want* to restrict the secure heap to be allocated from only by secure processes.
-Brian
linaro-mm-sig@lists.linaro.org
-
Benjamin Gaignard -
Brian Starkey -
Daniel Vetter -
Daniel Vetter -
Laura Abbott -
Rob Clark -
Robin Murphy