Re: [PATCH v2 1/2] irqchip/gic-v3-its: Share ITS tables with a non-trusted hypervisor

From: Steven Price
Date: Mon Oct 21 2024 - 06:52:58 EST


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@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Tested-by: Will Deacon <will@xxxxxxxxxx>
> Reviewed-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Steven Price <steven.price@xxxxxxx>
> ---
> 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<----