Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
---------------------------------------------------------------- Converts tee subsystem to use pin_user_pages() instead of get_user_pages()
---------------------------------------------------------------- John Hubbard (1): tee: convert get_user_pages() --> pin_user_pages()
drivers/tee/tee_shm.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
-Olof
On 8/21/20 11:49 AM, Olof Johansson wrote:
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
I think so. I had marked it in my notes as "accepted, and the maintainer will eventually merge it", and I left it at that. It's still desirable.
thanks,
On Fri, Aug 21, 2020 at 12:58 PM John Hubbard jhubbard@nvidia.com wrote:
On 8/21/20 11:49 AM, Olof Johansson wrote:
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
I think so. I had marked it in my notes as "accepted, and the maintainer will eventually merge it", and I left it at that. It's still desirable.
Looks like it conflicts with some of the later work. Jens, given the timelines here it's probably easiest all around if you rebase/respin and send a fresh pull request. I could fix it up but you'd still need to review that so the amount of work is probably less if you do it directly.
-Olof
On Fri, Aug 21, 2020 at 11:19 PM Olof Johansson olof@lixom.net wrote:
On Fri, Aug 21, 2020 at 12:58 PM John Hubbard jhubbard@nvidia.com wrote:
On 8/21/20 11:49 AM, Olof Johansson wrote:
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
I think so. I had marked it in my notes as "accepted, and the maintainer will eventually merge it", and I left it at that. It's still desirable.
Looks like it conflicts with some of the later work. Jens, given the timelines here it's probably easiest all around if you rebase/respin and send a fresh pull request. I could fix it up but you'd still need to review that so the amount of work is probably less if you do it directly.
Agree, I'll send a fresh pull request once we have this rebased. The conflict is with the recently added call to get_kernel_pages() when kernel memory is shared. The conflict isn't trivial, I guess we need to handle the different types of pages differently when releasing them. John, would you mind rebasing and posting the patch again?
Thanks, Jens
On 8/23/20 11:51 PM, Jens Wiklander wrote:
On Fri, Aug 21, 2020 at 11:19 PM Olof Johansson olof@lixom.net wrote:
On Fri, Aug 21, 2020 at 12:58 PM John Hubbard jhubbard@nvidia.com wrote:
On 8/21/20 11:49 AM, Olof Johansson wrote:
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
I think so. I had marked it in my notes as "accepted, and the maintainer will eventually merge it", and I left it at that. It's still desirable.
Looks like it conflicts with some of the later work. Jens, given the timelines here it's probably easiest all around if you rebase/respin and send a fresh pull request. I could fix it up but you'd still need to review that so the amount of work is probably less if you do it directly.
Agree, I'll send a fresh pull request once we have this rebased. The conflict is with the recently added call to get_kernel_pages() when kernel memory is shared. The conflict isn't trivial, I guess we need to handle the different types of pages differently when releasing them. John, would you mind rebasing and posting the patch again?
Sure. Should it be against 5.9-rc2, or something else? I can do this in the morning, about 10 hrs from now.
thanks,
On Mon, Aug 24, 2020 at 9:18 AM John Hubbard jhubbard@nvidia.com wrote:
On 8/23/20 11:51 PM, Jens Wiklander wrote:
On Fri, Aug 21, 2020 at 11:19 PM Olof Johansson olof@lixom.net wrote:
On Fri, Aug 21, 2020 at 12:58 PM John Hubbard jhubbard@nvidia.com wrote:
On 8/21/20 11:49 AM, Olof Johansson wrote:
On Tue, May 26, 2020 at 03:12:59PM +0200, Jens Wiklander wrote:
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use pin_user_pages() instead of get_user_pages().
Thanks, Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)Hi, I noticed this never got merged, but I don't see any follow-up here that retracts it. Is it still pending merge such that I should queue it for v5.10?
I think so. I had marked it in my notes as "accepted, and the maintainer will eventually merge it", and I left it at that. It's still desirable.
Looks like it conflicts with some of the later work. Jens, given the timelines here it's probably easiest all around if you rebase/respin and send a fresh pull request. I could fix it up but you'd still need to review that so the amount of work is probably less if you do it directly.
Agree, I'll send a fresh pull request once we have this rebased. The conflict is with the recently added call to get_kernel_pages() when kernel memory is shared. The conflict isn't trivial, I guess we need to handle the different types of pages differently when releasing them. John, would you mind rebasing and posting the patch again?
Sure. Should it be against 5.9-rc2, or something else? I can do this in the morning, about 10 hrs from now.
5.9-rc2 sounds good.
Thanks, Jens
This code was using get_user_pages*(), in a "Case 2" scenario (DMA/RDMA), using the categorization from [1]. That means that it's time to convert the get_user_pages*() + put_page() calls to pin_user_pages*() + unpin_user_pages() calls.
There is some helpful background in [2]: basically, this is a small part of fixing a long-standing disconnect between pinning pages, and file systems' use of those pages.
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages": https://lwn.net/Articles/807108/
Cc: Jens Wiklander jens.wiklander@linaro.org Cc: Sumit Semwal sumit.semwal@linaro.org Cc: tee-dev@lists.linaro.org Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: John Hubbard jhubbard@nvidia.com ---
OK, this should be indentical to v1 [1], but now rebased against Linux 5.9-rc2.
As before, I've compile-tested it again with a cross compiler, but that's the only testing I'm set up for with CONFIG_TEE.
[1] https://lore.kernel.org/r/20200519051850.2845561-1-jhubbard@nvidia.com
thanks, John Hubbard NVIDIA
drivers/tee/tee_shm.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 827ac3d0fea9..3c29e6c3ebe8 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -32,16 +32,13 @@ static void tee_shm_release(struct tee_shm *shm)
poolm->ops->free(poolm, shm); } else if (shm->flags & TEE_SHM_REGISTER) { - size_t n; int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
if (rc) dev_err(teedev->dev.parent, "unregister shm %p failed: %d", shm, rc);
- for (n = 0; n < shm->num_pages; n++) - put_page(shm->pages[n]); - + unpin_user_pages(shm->pages, shm->num_pages); kfree(shm->pages); }
@@ -228,7 +225,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, }
if (flags & TEE_SHM_USER_MAPPED) { - rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, + rc = pin_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages); } else { struct kvec *kiov; @@ -292,16 +289,13 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, return shm; err: if (shm) { - size_t n; - if (shm->id >= 0) { mutex_lock(&teedev->mutex); idr_remove(&teedev->idr, shm->id); mutex_unlock(&teedev->mutex); } if (shm->pages) { - for (n = 0; n < shm->num_pages; n++) - put_page(shm->pages[n]); + unpin_user_pages(shm->pages, shm->num_pages); kfree(shm->pages); } }
On 8/24/20 11:36 AM, John Hubbard wrote:
This code was using get_user_pages*(), in a "Case 2" scenario (DMA/RDMA), using the categorization from [1]. That means that it's time to convert the get_user_pages*() + put_page() calls to pin_user_pages*() + unpin_user_pages() calls.
There is some helpful background in [2]: basically, this is a small part of fixing a long-standing disconnect between pinning pages, and file systems' use of those pages.
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages": https://lwn.net/Articles/807108/
Cc: Jens Wiklander jens.wiklander@linaro.org Cc: Sumit Semwal sumit.semwal@linaro.org Cc: tee-dev@lists.linaro.org Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: John Hubbard jhubbard@nvidia.com
OK, this should be indentical to v1 [1], but now rebased against Linux 5.9-rc2.
...ohhh, wait, I should have read the earlier message from Jens more carefully:
"The conflict isn't trivial, I guess we need to handle the different types of pages differently when releasing them."
So it's not good to have a logically identical patch. argghhh. Let me see how hard it is to track these memory types separately and handle the release accordingly, just a sec.
Sorry about the false move here.
thanks,
This code was using get_user_pages*(), in a "Case 2" scenario (DMA/RDMA), using the categorization from [1]. That means that it's time to convert the get_user_pages*() + put_page() calls to pin_user_pages*() + unpin_user_pages() calls.
Factor out a new, small release_registered_pages() function, in order to consolidate the logic for discerning between TEE_SHM_USER_MAPPED and TEE_SHM_KERNEL_MAPPED pages. This also absorbs the kfree() call that is also required there.
There is some helpful background in [2]: basically, this is a small part of fixing a long-standing disconnect between pinning pages, and file systems' use of those pages.
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages": https://lwn.net/Articles/807108/
Cc: Jens Wiklander jens.wiklander@linaro.org Cc: Sumit Semwal sumit.semwal@linaro.org Cc: tee-dev@lists.linaro.org Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: John Hubbard jhubbard@nvidia.com ---
OK, one more try, this time actually handling the _USER_MAPPED vs. _KERNEL_MAPPED pages!
thanks, John Hubbard NVIDIA
drivers/tee/tee_shm.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 827ac3d0fea9..00472f5ce22e 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -12,6 +12,22 @@ #include <linux/uio.h> #include "tee_private.h"
+static void release_registered_pages(struct tee_shm *shm) +{ + if (shm->pages) { + if (shm->flags & TEE_SHM_USER_MAPPED) { + unpin_user_pages(shm->pages, shm->num_pages); + } else { + size_t n; + + for (n = 0; n < shm->num_pages; n++) + put_page(shm->pages[n]); + } + + kfree(shm->pages); + } +} + static void tee_shm_release(struct tee_shm *shm) { struct tee_device *teedev = shm->ctx->teedev; @@ -32,17 +48,13 @@ static void tee_shm_release(struct tee_shm *shm)
poolm->ops->free(poolm, shm); } else if (shm->flags & TEE_SHM_REGISTER) { - size_t n; int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
if (rc) dev_err(teedev->dev.parent, "unregister shm %p failed: %d", shm, rc);
- for (n = 0; n < shm->num_pages; n++) - put_page(shm->pages[n]); - - kfree(shm->pages); + release_registered_pages(shm); }
teedev_ctx_put(shm->ctx); @@ -228,7 +240,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, }
if (flags & TEE_SHM_USER_MAPPED) { - rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, + rc = pin_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages); } else { struct kvec *kiov; @@ -292,18 +304,12 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, return shm; err: if (shm) { - size_t n; - if (shm->id >= 0) { mutex_lock(&teedev->mutex); idr_remove(&teedev->idr, shm->id); mutex_unlock(&teedev->mutex); } - if (shm->pages) { - for (n = 0; n < shm->num_pages; n++) - put_page(shm->pages[n]); - kfree(shm->pages); - } + release_registered_pages(shm); } kfree(shm); teedev_ctx_put(ctx);
On Mon, Aug 24, 2020 at 02:11:25PM -0700, John Hubbard wrote:
This code was using get_user_pages*(), in a "Case 2" scenario (DMA/RDMA), using the categorization from [1]. That means that it's time to convert the get_user_pages*() + put_page() calls to pin_user_pages*() + unpin_user_pages() calls.
Factor out a new, small release_registered_pages() function, in order to consolidate the logic for discerning between TEE_SHM_USER_MAPPED and TEE_SHM_KERNEL_MAPPED pages. This also absorbs the kfree() call that is also required there.
There is some helpful background in [2]: basically, this is a small part of fixing a long-standing disconnect between pinning pages, and file systems' use of those pages.
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages": https://lwn.net/Articles/807108/
Cc: Jens Wiklander jens.wiklander@linaro.org Cc: Sumit Semwal sumit.semwal@linaro.org Cc: tee-dev@lists.linaro.org Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: John Hubbard jhubbard@nvidia.com
OK, one more try, this time actually handling the _USER_MAPPED vs. _KERNEL_MAPPED pages!
thanks, John Hubbard NVIDIA
Looks good and it works too! :-) I've tested it on my Hikey board with the OP-TEE test suite. I'm picking this up.
Thanks, Jens
On 8/25/20 1:32 AM, Jens Wiklander wrote:
On Mon, Aug 24, 2020 at 02:11:25PM -0700, John Hubbard wrote:
...
OK, one more try, this time actually handling the _USER_MAPPED vs. _KERNEL_MAPPED pages!
thanks, John Hubbard NVIDIA
Looks good and it works too! :-) I've tested it on my Hikey board with the OP-TEE test suite. I'm picking this up.
Great! I see that I have, once again, somehow doubled up on the subject line: "tee: convert convert ...". This particular typo just seems to stick to me. :)
If you get a chance to fix that up by changing it to just a single "convert" I'd appreciate it.
thanks,
On Tue, Aug 25, 2020 at 10:54 AM John Hubbard jhubbard@nvidia.com wrote:
On 8/25/20 1:32 AM, Jens Wiklander wrote:
On Mon, Aug 24, 2020 at 02:11:25PM -0700, John Hubbard wrote:
...
OK, one more try, this time actually handling the _USER_MAPPED vs. _KERNEL_MAPPED pages!
thanks, John Hubbard NVIDIA
Looks good and it works too! :-) I've tested it on my Hikey board with the OP-TEE test suite. I'm picking this up.
Great! I see that I have, once again, somehow doubled up on the subject line: "tee: convert convert ...". This particular typo just seems to stick to me. :)
If you get a chance to fix that up by changing it to just a single "convert" I'd appreciate it.
Sure, no problem.
Cheers, Jens
-
Jens Wiklander -
John Hubbard -
Olof Johansson