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

[Yong Wu (吴勇)]

On Mon, 2023-09-25 at 20:01 +0200, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> 
> 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@xxxxxxxxxxxx>
> >>> Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> >>> ---
> >>>    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_MT8173BIT(18)
> >>>    #define INT_ID_PORT_WIDTH_6BIT(19)
> >>>    #define CFG_IFA_MASTER_IN_ATFBIT(20)
> >>> +#define SECURE_BANK_ENABLEBIT(21)
> >>>    
> >>>    #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)\
> >>>    ((((pdata)->flags) & (mask)) == (_x))
> >>> @@ -162,6 +163,8 @@
> >>>    #define MTK_IOMMU_GROUP_MAX8
> >>>    #define MTK_IOMMU_BANK_MAX5
> >>>    
> >>> +#define MTK_IOMMU_SEC_BANKID4
> >>> +
> >>
> >> 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 don't think the 2 solutions is not equivalent. If we add a new
"banks_secure", for readability, we need add it for each current SoC.
This looks more complex. In current version I use a fixed value, which
is simpler, but of course lacks flexibility, which is what you are
worried about.

But we really have no plans to change this. Of course I can't be sure
what will happen in a few years. I think it's not complicated to
modify, let's modify if when necessary?

Thanks.

> 
> 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_tsec_bank_base;
> >>> +};
> >>>    intirq;
> >>>    u8id;
> >>> +boolis_secure;
> >>>    struct device*parent_dev;
> >>>    struct mtk_iommu_data*parent_data;
> >>>    spinlock_ttlb_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