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

[Steven Price]

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<----