Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 11/09/23 03:17, Yong Wu ha scritto:
> The lastest IOMMU always have 5 banks, and we always use the last bank
> (id:4) for the secure memory address translation. This patch add a new
> flag (SECURE_BANK_ENABLE) for this feature.
> 
> For the secure bank, its kernel va "base" is not helpful since the
> secure bank registers has already been protected and can only be accessed
> in the secure world. But we still record its register base, because we need
> use it to determine which IOMMU HW the translation fault happen in the
> secure world.
> 
> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>   drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 640275873a27..4a2cffb28c61 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -146,6 +146,7 @@
>   #define TF_PORT_TO_ADDR_MT8173		BIT(18)
>   #define INT_ID_PORT_WIDTH_6		BIT(19)
>   #define CFG_IFA_MASTER_IN_ATF		BIT(20)
> +#define SECURE_BANK_ENABLE		BIT(21)
>   
>   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
>   				((((pdata)->flags) & (mask)) == (_x))
> @@ -162,6 +163,8 @@
>   #define MTK_IOMMU_GROUP_MAX	8
>   #define MTK_IOMMU_BANK_MAX	5
>   
> +#define MTK_IOMMU_SEC_BANKID	4
> +

Is there any SoC (previous, current or future) that may have more than one
secure context bank?

I'm thinking about implementing this differently...

static const struct mtk_iommu_plat_data mt8188_data_vdo = {
	....
	.flags = ..flags.. | ATF_SECURE_BANKS_ENABLE
	.banks_num = 5,
	.banks_enable = {true, false, false, false, true},
	.banks_secure = {false, false, false, false, true},
	....
}

...this would means that you won't need to specify a static SEC_BANKID, as
you'd get that from banks_secure... so that....

>   enum mtk_iommu_plat {
>   	M4U_MT2712,
>   	M4U_MT6779,
> @@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
>   };
>   
>   struct mtk_iommu_bank_data {
> -	void __iomem			*base;
> +	union {
> +		void __iomem		*base;
> +		phys_addr_t		sec_bank_base;
> +	};
>   	int				irq;
>   	u8				id;
> +	bool				is_secure;
>   	struct device			*parent_dev;
>   	struct mtk_iommu_data		*parent_data;
>   	spinlock_t			tlb_lock; /* lock for tlb range flush */
> @@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   			continue;
>   		bank = &data->bank[i];
>   		bank->id = i;
> -		bank->base = base + i * MTK_IOMMU_BANK_SZ;

....this would become:

bank->is_secure = MTK_IOMMU_HAS_FLAG(data->plat_data, ATF_SECURE_BANKS_ENABLE) &&
		  data->plat_data->banks_secure[i];

if (bank->is_secure)
	bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
else
	bank->base = base + i * MTK_IOMMU_BANK_SZ;

> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, SECURE_BANK_ENABLE) &&
> +		    bank->id == MTK_IOMMU_SEC_BANKID) {
> +			/* Record the secure bank base to indicate which iommu TF in sec world */
> +			bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
> +			bank->is_secure = true;
> +		} else {
> +			bank->base = base + i * MTK_IOMMU_BANK_SZ;
> +			bank->is_secure = false;
> +		}
>   		bank->m4u_dom = NULL;
>   
>   		bank->irq = platform_get_irq(pdev, i);

What do you think?

Cheers,
Angelo