[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1f6e19c4-1fb9-43ab-a8a2-a465c9cff84b@arm.com>
Date: Mon, 21 Oct 2024 11:47:07 +0100
From: Steven Price <steven.price@....com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Will Deacon <will@...nel.org>, Suzuki K Poulose <suzuki.poulose@....com>,
Michael Kelley <mhklinux@...look.com>,
Shanker Donthineni <sdonthineni@...dia.com>, Marc Zyngier <maz@...nel.org>
Subject: Re: [PATCH v2 1/2] irqchip/gic-v3-its: Share ITS tables with a
non-trusted hypervisor
On 02/10/2024 15:16, Steven Price wrote:
> Within a realm guest the ITS is emulated by the host. This means the
> allocations must have been made available to the host by a call to
> set_memory_decrypted(). Introduce an allocation function which performs
> this extra call.
>
> For the ITT use a custom genpool-based allocator that calls
> set_memory_decrypted() for each page allocated, but then suballocates
> the size needed for each ITT. Note that there is no mechanism
> implemented to return pages from the genpool, but it is unlikely the
> peak number of devices will so much larger than the normal level - so
> this isn't expected to be an issue.
>
> Co-developed-by: Suzuki K Poulose <suzuki.poulose@....com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> Tested-by: Will Deacon <will@...nel.org>
> Reviewed-by: Marc Zyngier <maz@...nel.org>
> Signed-off-by: Steven Price <steven.price@....com>
> ---
> Changes since v1:
> * Drop WARN_ONs and add a comment explaining that, if they fail,
> set_memory_{en,de}crypted() will already have WARNed and that we are
> purposefully leaking memory in those cases.
> ---
> drivers/irqchip/irq-gic-v3-its.c | 151 ++++++++++++++++++++++++++-----
> 1 file changed, 128 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index fdec478ba5e7..7a62fd3a8673 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
<snip>
> +static void *itt_alloc_pool(int node, int size)
> +{
> + unsigned long addr;
> + struct page *page;
> +
> + if (size >= PAGE_SIZE) {
> + page = its_alloc_pages_node(node,
> + GFP_KERNEL | __GFP_ZERO,
> + get_order(size));
> +
> + if (!page)
> + return NULL;
> +
> + return page_address(page);
> + }
> +
> + do {
> + addr = gen_pool_alloc(itt_pool, size);
> + if (addr)
> + break;
> +
> + page = its_alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 1);
Shanker pointed out[1] that this line is wrong. It's allocating 2 pages
not the expect single page because the final argument is the order and
1<<1 is 2.
Can you squash in (or apply as a fix) the following?
Thanks,
Steve
[1] https://lore.kernel.org/r/ed65312a-245c-4fa5-91ad-5d620cab7c6b%40nvidia.com
----8<----
>From ada4dc7b3e028a9901219fe0f9212d665a4e610b Mon Sep 17 00:00:00 2001
From: Steven Price <steven.price@....com>
Date: Mon, 21 Oct 2024 11:41:05 +0100
Subject: [PATCH] irqchip/gic-v3-its: Fix over allocation in itt_alloc_pool()
itt_alloc_pool() calls its_alloc_pages_node() to allocate an individual
page to add to the pool (for allocations <PAGE_SIZE). However the final
argument of its_alloc_pages_node() is the page order not the number of
pages. Currently it allocates two pages and leaks the second page.
Fix it by passing 0 instead (1 << 0 = 1 page).
Reported-by: Shanker Donthineni <sdonthineni@...dia.com>
Signed-off-by: Steven Price <steven.price@....com>
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6c1581bf1ae0..526be60e3502 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -261,7 +261,7 @@ static void *itt_alloc_pool(int node, int size)
if (addr)
break;
- page = its_alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 1);
+ page = its_alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
if (!page)
break;
--
2.34.1
Powered by blists - more mailing lists