Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <869d9122-6187-47a0-0589-9feeafaf984c@baylibre.com>
Date:   Mon, 25 Sep 2023 20:01:12 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank
Content-Language: en-US
To:     =?UTF-8?B?WW9uZyBXdSAo5ZC05YuHKQ==?= <Yong.Wu@mediatek.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "will@kernel.org" <will@kernel.org>,
        "angelogioacchino.delregno@collabora.com" 
        <angelogioacchino.delregno@collabora.com>,
        "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        =?UTF-8?B?QW5hbiBTdW4gKOWtmeWuieWuiSk=?= <Anan.Sun@mediatek.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        =?UTF-8?B?WUYgV2FuZyAo546L5LqR6aOeKQ==?= <YF.Wang@mediatek.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "tjmercier@google.com" <tjmercier@google.com>,
        =?UTF-8?B?TWluZ3l1YW4gTWEgKOmprOm4o+i/nCk=?= 
        <Mingyuan.Ma@mediatek.com>
References: <20230911011751.29906-1-yong.wu@mediatek.com>
 <20230911011751.29906-2-yong.wu@mediatek.com>
 <28b81a20-5cfa-b474-41c3-c01b7b846e21@collabora.com>
 <409168b540948d312a91022f375fa71a470e9d60.camel@mediatek.com>
From:   Alexandre Mergnat <amergnat@baylibre.com>
In-Reply-To: <409168b540948d312a91022f375fa71a470e9d60.camel@mediatek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 25/09/2023 14:50, Yong Wu (吴勇) wrote:
> On Mon, 2023-09-11 at 11:22 +0200, AngeloGioacchino Del Regno wrote:
>> 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?
> 
> Thanks very much for the below detail suggestion. But No, for MM IOMMU,
> The bank4 is mandatory the secure bank, and there is only this one
> secure bank, and this is the case for all the current projects, we have
> no plan to modify this at the moment. Therefore I think a macro is ok
> for it.
> 

Between 2 solutions which have the equivalent complexity (logical & 
readability), I prefer the most generic one (at least for generic 
drivers like this). Nobody is aware about future SoC, even if you know 
what will have the next SoC generation, I'm not sure you can certified 
it will be the same in the next 2, 3, 4,... generations.

I'm convinced it will be easier in the future to maintain the IOMMU code 
if it's flexible.

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

-- 
Regards,
Alexandre