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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <28b81a20-5cfa-b474-41c3-c01b7b846e21@collabora.com>
Date:   Mon, 11 Sep 2023 11:22:56 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Yong Wu <yong.wu@...iatek.com>, Joerg Roedel <joro@...tes.org>,
        Will Deacon <will@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>
Cc:     Robin Murphy <robin.murphy@....com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        iommu@...ts.linux.dev, linux-mediatek@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        anan.sun@...iatek.com, yf.wang@...iatek.com,
        mingyuan.ma@...iatek.com, "T . J . Mercier" <tjmercier@...gle.com>
Subject: Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

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@...iatek.com>
> Signed-off-by: Yong Wu <yong.wu@...iatek.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