mbox series

[v1,0/7] Refactor the SMMU's CD table ownership

Message ID 20230727182647.4106140-1-mshavit@google.com
Headers
Series Refactor the SMMU's CD table ownership |

Message

Michael Shavit July 27, 2023, 6:26 p.m. UTC 
  Hi all,

This series refactors stage 1 domains so that they describe a single CD
entry. These entries are now inserted into a CD table that is owned by
the arm_smmu_master instead of the domain.
This is conceptually cleaner and unblocks other features, such as
attaching domains with PASID (for unmanaged/dma domains).

This patch series was originally part of a larger patch series that
implemented the set_dev_pasid callback for non-SVA domains but is now
split into a distinct series.

This patch series is also available on gerrit with Jean's SMMU test
engine patches cherry-picked on top for testing:
https://linux-review.git.corp.google.com/c/linux/kernel/git/torvalds/linux/+/24729

Thanks,
Michael Shavit

Changelog
v1:
* Replace s1_cfg with arm_smmu_ctx_desc_cfg representing the CD table
* Assume that the CD table is owned by the SMMU master for most
  operations. This is forward-compatible with the nested patch series as
  these operations wouldn't be called when the installed CD table comes
  from nested domains.
* Split off as a distinct patch series Split-off from:
https://lore.kernel.org/all/20230621063825.268890-1-mshavit@google.com/

Michael Shavit (7):
  iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
  iommu/arm-smmu-v3: Replace s1_cfg with ctx_desc_cfg
  iommu/arm-smmu-v3: Encapsulate ctx_desc_cfg init in alloc_cd_tables
  iommu/arm-smmu-v3: move stall_enabled to the cd table
  iommu/arm-smmu-v3: Skip cd sync if CD table isn't active
  iommu/arm-smmu-v3: Refactor write_ctx_desc
  iommu/arm-smmu-v3: Move CD table to arm_smmu_master

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  35 +++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 167 ++++++++----------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  39 ++--
 3 files changed, 123 insertions(+), 118 deletions(-)


base-commit: 0a8db05b571ad5b8d5c8774a004c0424260a90bd

Comments

Jason Gunthorpe July 27, 2023, 6:33 p.m. UTC | #1 
On Fri, Jul 28, 2023 at 02:26:21AM +0800, Michael Shavit wrote:
> This commit explicitly keeps track of whether a CD table is installed in
> an STE so that arm_smmu_sync_cd can skip the sync when unnecessary. This
> was previously achieved through the domain->devices list, but we are
> moving to a model where arm_smmu_sync_cd directly operates on a master
> and the master's CD table instead of a domain.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++
>  2 files changed, 6 insertions(+)
> 
> 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 654acf6002bf3..af7949b62327b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -987,6 +987,9 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
>  		},
>  	};
>  
> +	if (!smmu_domain->cd_table.installed)
> +		return;
> +
>  	cmds.num = 0;
>  
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> @@ -1368,6 +1371,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>  				  cd_table->l1_desc ?
>  					  STRTAB_STE_0_S1FMT_64K_L2 :
>  					  STRTAB_STE_0_S1FMT_LINEAR);
> +		cd_table->installed = true;
>  	}
>  
>  	if (s2_cfg) {
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 0e55ca0d40e6b..f301efe90b599 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -599,6 +599,8 @@ struct arm_smmu_ctx_desc_cfg {
>  	u8				max_cds_bits;
>  	/* Whether CD entries in this table have the stall bit set. */
>  	bool				stall_enabled;
> +	/* Whether this CD table is installed in any STE */
> +	bool                            installed;

Do

u8 xx:1;
u8 yy:1;

For these things, Linus has complained about lists of bools in structs
before.

Jason
Jason Gunthorpe July 27, 2023, 6:45 p.m. UTC | #2 
On Fri, Jul 28, 2023 at 02:26:17AM +0800, Michael Shavit wrote:
> @@ -715,25 +714,28 @@ enum arm_smmu_domain_stage {
>  };
>  
>  struct arm_smmu_domain {
> -	struct arm_smmu_device		*smmu;
> -	struct mutex			init_mutex; /* Protects smmu pointer */
> +	struct arm_smmu_device			*smmu;
> +	struct mutex				init_mutex; /* Protects smmu pointer */
>  
> -	struct io_pgtable_ops		*pgtbl_ops;
> -	bool				stall_enabled;
> -	atomic_t			nr_ats_masters;
> +	struct io_pgtable_ops			*pgtbl_ops;
> +	bool					stall_enabled;
> +	atomic_t				nr_ats_masters;
>  
> -	enum arm_smmu_domain_stage	stage;
> +	enum arm_smmu_domain_stage		stage;
>  	union {
> -		struct arm_smmu_s1_cfg	s1_cfg;
> -		struct arm_smmu_s2_cfg	s2_cfg;
> +		struct {
> +		struct arm_smmu_ctx_desc	cd;
> +		struct arm_smmu_s1_cfg		s1_cfg;
> +		};
> +		struct arm_smmu_s2_cfg		s2_cfg;
>  	};
>  
> -	struct iommu_domain		domain;
> +	struct iommu_domain			domain;
>  
> -	struct list_head		devices;
> -	spinlock_t			devices_lock;
> +	struct list_head			devices;
> +	spinlock_t				devices_lock;
>  
> -	struct list_head		mmu_notifiers;
> +	struct list_head			mmu_notifiers;
>  };

Don't re-indent a whole struct just to get column alignment. Do
a few lines around where you are touching.

This is also why I quite dislike column alignment, aside from being
unreadable, it harms readability of diffs and increases maintenance
costs.

Jason
Nicolin Chen July 27, 2023, 8:19 p.m. UTC | #3 
Hi Michael,

Thanks for sending this!

On Fri, Jul 28, 2023 at 02:26:16AM +0800, Michael Shavit wrote:
 
> This series refactors stage 1 domains so that they describe a single CD
> entry. These entries are now inserted into a CD table that is owned by
> the arm_smmu_master instead of the domain.
> This is conceptually cleaner and unblocks other features, such as
> attaching domains with PASID (for unmanaged/dma domains).
> 
> This patch series was originally part of a larger patch series that
> implemented the set_dev_pasid callback for non-SVA domains but is now
> split into a distinct series.
> 
> This patch series is also available on gerrit with Jean's SMMU test
> engine patches cherry-picked on top for testing:
> https://linux-review.git.corp.google.com/c/linux/kernel/git/torvalds/linux/+/24729

The link isn't accessible for public. I guess it should be this?
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24729

Nicolin
Michael Shavit July 28, 2023, 7:18 a.m. UTC | #4 
On Fri, Jul 28, 2023 at 4:20 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> The link isn't accessible for public. I guess it should be this?
> https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24729

Whoops sorry yeah that's the correct link.