Re: [PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Mostafa Saleh <smostafa@google.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>, Eric Auger <eric.auger@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Moritz Fischer <mdf@kernel.org>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr()
Date: Mon, 25 Mar 2024 11:21:28 -0300	[thread overview]
Message-ID: <20240325142128.GC110546@nvidia.com> (raw)
In-Reply-To: <Zf3W3lo8dXZuKbgm@google.com>

On Fri, Mar 22, 2024 at 07:07:10PM +0000, Mostafa Saleh wrote:
> Hi Jason,
> 
> On Mon, Mar 04, 2024 at 07:43:56PM -0400, Jason Gunthorpe wrote:
> > No reason to force callers to do two steps. Make arm_smmu_get_cd_ptr()
> > able to return an entry in all cases except OOM
>
> I believe the current code is more clear, as it is explicit about which path
> is expected to allocate.

I think we had this allocate vs no allocate discussion before on
something else..

It would be good to make *full* allocate/noallocate variants of
get_cd_ptr() and the cases that must never allocate call the no
allocate variation. There are some issues with GFP_KERNEL/ATOMIC that
are a bit hard to understand as well.

This is a bigger issue than just the cd_table, as even the leafs
should not allocate.

> As there are many callers for arm_smmu_get_cd_ptr() directly and indirectly,
> and it read-modify-writes the cdtable, it would be a pain to debug not
> knowing which one could allocate, and this patch only abstracts one
> allocating call, so it is not much code less.

> For example, (again I don’t know much about SVA) I think there might be a
> race condition as follows:
> arm_smmu_attach_dev
> 	arm_smmu_domain_finalise() => set domain stage
> 	[....]
> 	arm_smmu_get_cd_ptr() => RMW master->cd_table
> 
> arm_smmu_sva_set_dev_pasid
> 	__arm_smmu_sva_bind
> 		Check stage is valid
> 		[...]
> 		arm_smmu_write_ctx_desc
> 			arm_smmu_get_cd_ptr => RMW master->cd_table
> 
> If this path is true though, I guess the in current code, we would need some
> barriers in arm_smmu_get_cd_ptr(), arm_smmu_get_cd_ptr()

Both of those functions are called under the group mutex held by the
core code.

The only valid condition to change the CD table backing memory is when
the group mutex is held. We now have iommu_group_mutex_assert() so an
alloc/noalloc split can call that test on the alloc side which is
motivation enough to do it, IMHO.

I will split the function and sort it all out, but I will still
integrate the cd_table allocation into the allocating version of
get_cd_ptr().

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel