diff mbox series

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

Message ID 8-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Update SMMUv3 to the modern iommu API (part 2/3) | expand

Commit Message

Jason Gunthorpe March 4, 2024, 11:43 p.m. UTC 
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.

Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Michael Shavit March 13, 2024, 12:15 p.m. UTC | #1 
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> 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.
>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index e25dbb982feeee..2dd6cb17112e98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -106,6 +106,7 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>                                     struct arm_smmu_device *smmu);
> +static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>
>  static void parse_driver_options(struct arm_smmu_device *smmu)
>  {
> @@ -1231,6 +1232,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>         struct arm_smmu_device *smmu = master->smmu;
>         struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>
> +       if (!master->cd_table.cdtab) {
> +               if (arm_smmu_alloc_cd_tables(master))
> +                       return NULL;
> +       }
> +
>         if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>                 return (struct arm_smmu_cd *)(cd_table->cdtab +
>                                               ssid * CTXDESC_CD_DWORDS);
> @@ -2719,12 +2725,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                 struct arm_smmu_cd target_cd;
>                 struct arm_smmu_cd *cdptr;
>
> -               if (!master->cd_table.cdtab) {
> -                       ret = arm_smmu_alloc_cd_tables(master);
> -                       if (ret)
> -                               goto out_list_del;
> -               }
> -
>                 cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
>                 if (!cdptr) {
>                         ret = -ENOMEM;
> --
> 2.43.2
>
Reviewed-by: Michael Shavit <mshavit@google.com>
Nicolin Chen March 16, 2024, 3:31 a.m. UTC | #2 
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.
> 
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
 
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Mostafa Saleh March 22, 2024, 7:07 p.m. UTC | #3 
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.

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()

> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index e25dbb982feeee..2dd6cb17112e98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -106,6 +106,7 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  				    struct arm_smmu_device *smmu);
> +static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>  
>  static void parse_driver_options(struct arm_smmu_device *smmu)
>  {
> @@ -1231,6 +1232,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>  	struct arm_smmu_device *smmu = master->smmu;
>  	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>  
> +	if (!master->cd_table.cdtab) {
> +		if (arm_smmu_alloc_cd_tables(master))
> +			return NULL;
> +	}
> +
>  	if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>  		return (struct arm_smmu_cd *)(cd_table->cdtab +
>  					      ssid * CTXDESC_CD_DWORDS);
> @@ -2719,12 +2725,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		struct arm_smmu_cd target_cd;
>  		struct arm_smmu_cd *cdptr;
>  
> -		if (!master->cd_table.cdtab) {
> -			ret = arm_smmu_alloc_cd_tables(master);
> -			if (ret)
> -				goto out_list_del;
> -		}
> -
>  		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
>  		if (!cdptr) {
>  			ret = -ENOMEM;
> -- 
> 2.43.2
>
Thanks,
Mostafa
Jason Gunthorpe March 25, 2024, 2:21 p.m. UTC | #4 
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
Mostafa Saleh March 25, 2024, 9:03 p.m. UTC | #5 
On Mon, Mar 25, 2024 at 11:21:28AM -0300, Jason Gunthorpe wrote:
> 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().

I prefer that, as it makes it clear which paths expect to allocate
and which not.

> Thanks,
> Jason

Thanks,
Mostafa
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e25dbb982feeee..2dd6cb17112e98 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -106,6 +106,7 @@  static struct arm_smmu_option_prop arm_smmu_options[] = {
 
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 				    struct arm_smmu_device *smmu);
+static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
@@ -1231,6 +1232,11 @@  struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
 	struct arm_smmu_device *smmu = master->smmu;
 	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
 
+	if (!master->cd_table.cdtab) {
+		if (arm_smmu_alloc_cd_tables(master))
+			return NULL;
+	}
+
 	if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
 		return (struct arm_smmu_cd *)(cd_table->cdtab +
 					      ssid * CTXDESC_CD_DWORDS);
@@ -2719,12 +2725,6 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		struct arm_smmu_cd target_cd;
 		struct arm_smmu_cd *cdptr;
 
-		if (!master->cd_table.cdtab) {
-			ret = arm_smmu_alloc_cd_tables(master);
-			if (ret)
-				goto out_list_del;
-		}
-
 		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
 		if (!cdptr) {
 			ret = -ENOMEM;