RE: [PATCH 2/4] tee: Remove vmalloc page support

[Phil Chang (張世勳)]

Hi Sumit

Thanks for mentioning that, in fact, our product is low memory devices, and continuous pages are extremely valuable.
Although our driver is not upstream yet but highly dependent on tee shm vmalloc support,
some scenarios are driver alloc high order pages but system memory is fragmentation so that alloc failed.
In this situation, vmalloc support is important and gives flexible usage to user.


-----Original Message-----
From: Sumit Garg <sumit.garg@xxxxxxxxxx> 
Sent: Monday, October 3, 2022 2:57 PM
To: ira.weiny@xxxxxxxxx
Cc: Jens Wiklander <jens.wiklander@xxxxxxxxxx>; Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; Al Viro <viro@xxxxxxxxxxxxxxxxxx>; Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>; Christoph Hellwig <hch@xxxxxx>; Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>; op-tee@xxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; Phil Chang (張世勳) <Phil.Chang@xxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] tee: Remove vmalloc page support

+ Phil

Hi Ira,

On Sun, 2 Oct 2022 at 05:53, <ira.weiny@xxxxxxxxx> wrote:
>
> From: Ira Weiny <ira.weiny@xxxxxxxxx>
>
> The kernel pages used by shm_get_kernel_pages() are allocated using 
> GFP_KERNEL through the following call stack:
>
> trusted_instantiate()
>         trusted_payload_alloc() -> GFP_KERNEL
>         <trusted key op>
>                 tee_shm_register_kernel_buf()
>                         register_shm_helper()
>                                 shm_get_kernel_pages()
>
> Where <trusted key op> is one of:
>
>         trusted_key_unseal()
>         trusted_key_get_random()
>         trusted_key_seal()
>
> Remove the vmalloc page support from shm_get_kernel_pages().  Replace 
> with a warn on once.
>
> Cc: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: "Fabio M. De Francesco" <fmdefrancesco@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
>
> ---
> Jens I went with the suggestion from Linus and Christoph and rejected 
> vmalloc addresses.  I did not hear back from you regarding Linus'
> question if the vmalloc page support was required by an up coming 
> patch set or not.  So I assumed it was something out of tree.

It looks like I wasn't CC'd to that conversation. IIRC, support for vmalloc addresses was added recently by Phil here [1]. So I would like to give him a chance if he is planning to post a corresponding kernel driver upstream.

[1] https://urldefense.com/v3/__https://lists.trustedfirmware.org/archives/list/op-tee@xxxxxxxxxxxxxxxxxxxxxxxxx/thread/M7HI3P2M66V27SK35CGQRICZ7DJZ5J2W/__;!!CTRNKA9wMg0ARbw!wGOKR9k3khZJlPt1K_xBCXX4EBM5ZCfWKuruFgSP45H8wTvJrx4_St3Fb5ZrljD5QQ$ 

-Sumit

> ---
>  drivers/tee/tee_shm.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 
> 27295bda3e0b..527a6eabc03e 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -24,37 +24,25 @@ static void shm_put_kernel_pages(struct page 
> **pages, size_t page_count)  static int shm_get_kernel_pages(unsigned long start, size_t page_count,
>                                 struct page **pages)  {
> +       struct kvec *kiov;
>         size_t n;
>         int rc;
>
> -       if (is_vmalloc_addr((void *)start)) {
> -               struct page *page;
> -
> -               for (n = 0; n < page_count; n++) {
> -                       page = vmalloc_to_page((void *)(start + PAGE_SIZE * n));
> -                       if (!page)
> -                               return -ENOMEM;
> -
> -                       get_page(page);
> -                       pages[n] = page;
> -               }
> -               rc = page_count;
> -       } else {
> -               struct kvec *kiov;
> -
> -               kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> -               if (!kiov)
> -                       return -ENOMEM;
> +       if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
> +               return -EINVAL;
>
> -               for (n = 0; n < page_count; n++) {
> -                       kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> -                       kiov[n].iov_len = PAGE_SIZE;
> -               }
> +       kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> +       if (!kiov)
> +               return -ENOMEM;
>
> -               rc = get_kernel_pages(kiov, page_count, 0, pages);
> -               kfree(kiov);
> +       for (n = 0; n < page_count; n++) {
> +               kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> +               kiov[n].iov_len = PAGE_SIZE;
>         }
>
> +       rc = get_kernel_pages(kiov, page_count, 0, pages);
> +       kfree(kiov);
> +
>         return rc;
>  }
>
> --
> 2.37.2
>