Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Steven Price <steven.price@arm.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Heiko Stuebner <heiko@sntech.de>, Joerg Roedel <jroedel@suse.de>,
	Jerry Snitselaar <jsnitsel@redhat.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>
Subject: Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
Date: Fri, 6 Oct 2023 13:23:27 -0300	[thread overview]
Message-ID: <20231006162327.GA1132146@nvidia.com> (raw)
In-Reply-To: <61ef049f-8c40-47ec-b3fd-d17cbe41b96b@arm.com>

On Fri, Oct 06, 2023 at 04:11:22PM +0100, Steven Price wrote:
> On 05/10/2023 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >  	u32 fsr, fsynr, cbfrsynra;
> >  	unsigned long iova;
> >  	struct iommu_domain *domain = dev;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +	struct arm_smmu_domain *smmu_domain = dev;
> 
> Leaving aside Robin's objections - this change is clearly bogus. 'dev'
> is now being case to both struct iommu_domain and struct
> arm_smmu_domain. And AFAICT that won't even "happen to work" because the
> struct iommu_domain isn't the first element of struct
> arm_smmu_domain.

Oh I totally missed that! I fixed it like this:

--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -392,7 +392,6 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 {
        u32 fsr, fsynr, cbfrsynra;
        unsigned long iova;
-       struct iommu_domain *domain = dev;
        struct arm_smmu_domain *smmu_domain = dev;
        struct arm_smmu_device *smmu = smmu_domain->smmu;
        int idx = smmu_domain->cfg.cbndx;
@@ -406,7 +405,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
        iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
        cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
 
-       ret = report_iommu_fault(domain, NULL, iova,
+       ret = report_iommu_fault(&smmu_domain->domain, NULL, iova,
                fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);

Thanks!
Jason
