Re: [PATCH v2 1/2] iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K

[Will Deacon <will@kernel.org>]

On Mon, Dec 02, 2024 at 02:06:03PM +0000, Mostafa Saleh wrote:
> At the moment, io-pgtable-arm uses concatenation only if it is
> possible at level 0, which misses a case where concatenation is
> mandatory at level 1 according to R_SRKBC in Arm spec DDI0487 K.a.
> 
> Also, that means concatenation can be used when not mandated,
> contradicting the comment on the code. However, these cases can only
> happen if the SMMUv3 driver is changed to use ias != oas for stage-2.
> 
> This patch re-writes the code to use concatenation only if mandatory,
> fixing the missing case for level-1 and granule 16K with PA = 40 bits.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  drivers/iommu/io-pgtable-arm.c | 45 +++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 6b9bb58a414f..0055876b3527 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -223,6 +223,33 @@ static inline int arm_lpae_max_entries(int i, struct arm_lpae_io_pgtable *data)
>  	return ptes_per_table - (i & (ptes_per_table - 1));
>  }
>  
> +/*
> + * Check if concatenated PGDs are mandatory according to Arm DDI0487 (K.a)
> + * 1) R_DXBSH: For 16KB, and 48-bit input size, use level 1 instead of 0.
> + * 2) R_SRKBC: After de-ciphering the table for PA size and valid initial lookup
> + *   a) 40 bits PA size with 4K: use level 1 instead of level 0 (2 tables for ias = oas)
> + *   b) 40 bits PA size with 16K: use level 2 instead of level 1 (16 tables for ias = oas)
> + *   c) 42 bits PA size with 4K: use level 1 instead of level 0 (8 tables for ias = oas)
> + *   d) 48 bits PA size with 16K: use level 1 instead of level 0 (2 tables for ias = oas)
> + */
> +static inline bool arm_lpae_concat_mandatory(struct arm_lpae_io_pgtable *data)
> +{
> +	unsigned int ias = data->iop.cfg.ias;
> +	unsigned int oas = data->iop.cfg.oas;
> +
> +	/* Covers 1  and 2.d */
> +	if ((ARM_LPAE_GRANULE(data) == SZ_16K) && (data->start_level == 0))
> +		return (oas == 48) || (ias = 48);

I'm guessing the second disjunct here should be:

	(ias == 48);

I'll make that change when applying...

Will