linux-kernel - Re: [PATCH v4 2/4] iommu/mediatek: Adjust mtk_iommu

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <65be8fa931847d690b3cfa1a97155bb599656e33.camel@mediatek.com>
Date:   Mon, 21 Nov 2022 06:37:22 +0000
From:   Chengci Xu (许承赐) 
        <Chengci.Xu@...iatek.com>
To:     Yong Wu (吴勇) <Yong.Wu@...iatek.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "joro@...tes.org" <joro@...tes.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH v4 2/4] iommu/mediatek: Adjust mtk_iommu_config flow

On Mon, 2022-11-21 at 05:17 +0000, Yong Wu (吴勇) wrote:
> On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> > For reduce indention without functional change, prepare for MT8188.
> > If there are many port in a same larb, current flow will update
> > larb_mmu->mmu or update INFRA register for too many times.
> > So we save all port to portid_msk in the front of
> > mtk_iommu_config(),
> > and then update only once for IOMMU configure. By this
> > modification,
> > we can prevent MT8188 from sending to many SMC calls, avoiding
> > enter
> > ATF for each port.
> > 
> > Signed-off-by: Chengci.Xu <chengci.xu@...iatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c | 60 ++++++++++++++++++++++-------------
> > --
> > --
> >  1 file changed, 34 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 5a4e00e4bbbc..fbaf401f34e0 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -534,41 +534,49 @@ static int mtk_iommu_config(struct
> > mtk_iommu_data *data, struct device *dev,
> >  	unsigned int                 larbid, portid;
> >  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >  	const struct mtk_iommu_iova_region *region;
> > -	u32 peri_mmuen, peri_mmuen_msk;
> > +	unsigned long portid_msk_ext;
> > +	u32 portid_msk = 0;
> >  	int i, ret = 0;
> >  
> >  	for (i = 0; i < fwspec->num_ids; ++i) {
> > -		larbid = MTK_M4U_TO_LARB(fwspec->ids[i]);
> >  		portid = MTK_M4U_TO_PORT(fwspec->ids[i]);
> > +		portid_msk |= BIT(portid);
> > +	}
> >  
> > -		if (MTK_IOMMU_IS_TYPE(data->plat_data,
> > MTK_IOMMU_TYPE_MM)) {
> > -			larb_mmu = &data->larb_imu[larbid];
> > +	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
> > +		/* All ports should be in the same larb. just use 0
> > here */
> > +		larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> > +		larb_mmu = &data->larb_imu[larbid];
> > +		region = data->plat_data->iova_region + regionid;
> >  
> > -			region = data->plat_data->iova_region +
> > regionid;
> > +		portid_msk_ext = portid_msk;
> > +		for_each_set_bit(portid, &portid_msk_ext, 32)
> 
> Why do we need define a new portid_msk_ext? Can't we use portid_msk
> directly?

Thanks for your review.

The second parameter of for_each_set_bit is an address of "ulong",
which is shown as "const unsigned long *", but portid_msk is "u32".

I have tried following two solutions to get correct address of ulong
from portid_msk:
(1) (unsigned long *)&portid_msk
	If we get the address of portid_msk by "&" and cast it to
"unsigned long *", "build error will happened. The fail reason we can
find in build_allmodconfig.arm64.log is"
/tmp/src_kernel/kernel/linux-next/drivers/iommu/mtk_iommu.c: In
function 'mtk_iommu_config': /tmp/src_kernel/kernel/linux-
next/include/linux/find.h:58:23: error: array subscript 'long unsigned
int[0]' is partly outside array bounds of 'u32[1]' {aka 'unsigned
int[1]'} [-Werror=array-bounds]
   58 |                 val = *addr & GENMASK(size - 1, offset);
(2) &((unsigned long)portid_msk)
	This is not allowed beacuse "(unsigned long)portid_msk" is a
right value and geting the address of a right value is illegal.

So I choose to define a new variable "portid_msk_ext" whose type is
"unsigned long". I know it is a ugly soultion just to make function ok
and build pass, but it's hard for me to catch up with other soultions.

May be we can change the type of portid_msk from "u32" to "u64", is
this OK for you?