[RFC,v1,0/8] Install domain onto multiple smmus

  Hi all,

This series refactors the arm-smmu-v3 driver to support attaching
domains onto masters belonging to different smmu devices.

The main objective of this series is allow further refactorings of
arm-smmu-v3-sva. Specifically, we'd like to reach the state where:
1. A single SVA domain is allocated per MM/ASID
2. arm-smmu-v3-sva's set_dev_pasid implementation directly attaches that
   SVA domain to different masters, regardless of whether those masters
   belong to different smmus.

If armm-smmu-v3-sva is handed iommu_domains that have a 1:1 relationship
with an MM struct, then it won't have to share a CD with multiple
domains (or arm_smmu_mmu_notifiers). But to get there, the arm-smmu-v3
driver must first support domains installed on multiple SMMU devices.

This series depends on the CD table ownership refactor: https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/
as well as the VMID IDA patch: https://lore.kernel.org/all/169087904450.1290857.11726985177314533259.b4-ty@kernel.org/#r

This series is also available on gerrit: https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24829/6

Thanks,
Michael Shavit


Michael Shavit (8):
  iommu/arm-smmu-v3: Add list of installed_smmus
  iommu/arm-smmu-v3: Perform invalidations over installed_smmus
  iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus
  iommu/arm-smmu-v3: check smmu compatibility on attach
  iommu/arm-smmu-v3: Add arm_smmu_device as a parameter to
    domain_finalise
  iommu/arm-smmu-v3: Free VMID when uninstalling domain from SMMU
  iommu/arm-smmu-v3: check for domain initialization using pgtbl_ops
  iommu/arm-smmu-v3: allow multi-SMMU domain installs.

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  62 +++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 348 +++++++++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  21 +-
 3 files changed, 320 insertions(+), 111 deletions(-)


base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
prerequisite-patch-id: f701e5ac2cce085366342edff287a35d1cb82b9c
prerequisite-patch-id: c8d21ff19c2c1dd18799a6b83f483add654d187e
prerequisite-patch-id: 6ebba95cb12a723645843b4bd1bc45c94779d853
prerequisite-patch-id: 3f767e1c37d2996323c4f6d2a2d1912ab75281f7
prerequisite-patch-id: 5a4109fa3e22e2399ad064951c2ca1aeba4a68f7
prerequisite-patch-id: c4b3bd34b8be7afebd3e44bc4ec218d74753ce77
prerequisite-patch-id: 6d89e53518d25ac983ac99786950ee1a558c271f
prerequisite-patch-id: 447219e565cadc34b03db05dad58d8e5c4b5a382
prerequisite-patch-id: 63adb2c3f97d4948d96a0d5960184f5ac814d7f7
prerequisite-patch-id: e71195fcf1aa56d8ef9d7403b9e4492c17b8fb84
prerequisite-patch-id: ba82add44850bf8fb271292020edb746aef93a65
  

On 2023-08-17 19:16, Michael Shavit wrote:
> Record the domain's pgtbl_cfg when it's being prepared so that it can
> later be compared to the features an smmu supports.

What's wrong with retrieving the existing config from the 
io_pgtable_ops, same as all the other io-pgtable code does?

Thanks,
Robin.

> Verify a domain's compatibility with the smmu when it's being attached
> to a master belong to a different smmu device.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 103 ++++++++++++++++----
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   2 +
>   2 files changed, 86 insertions(+), 19 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 447af74dbe280..c0943cf3c09ca 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2195,17 +2195,48 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
>   	return 0;
>   }
>   
> +static int arm_smmu_prepare_pgtbl_cfg(struct arm_smmu_device *smmu,
> +				      enum arm_smmu_domain_stage stage,
> +				      struct io_pgtable_cfg *pgtbl_cfg)
> +{
> +	unsigned long ias, oas;
> +
> +	switch (stage) {
> +	case ARM_SMMU_DOMAIN_S1:
> +		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> +		ias = min_t(unsigned long, ias, VA_BITS);
> +		oas = smmu->ias;
> +		break;
> +	case ARM_SMMU_DOMAIN_NESTED:
> +	case ARM_SMMU_DOMAIN_S2:
> +		ias = smmu->ias;
> +		oas = smmu->oas;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.ias		= ias,
> +		.oas		= oas,
> +		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +	return 0;
> +}
> +
>   static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   {
>   	int ret;
> -	unsigned long ias, oas;
>   	enum io_pgtable_fmt fmt;
> -	struct io_pgtable_cfg pgtbl_cfg;
>   	struct io_pgtable_ops *pgtbl_ops;
>   	int (*finalise_stage_fn)(struct arm_smmu_domain *,
>   				 struct io_pgtable_cfg *);
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
> +	struct io_pgtable_cfg *pgtbl_cfg = &smmu_domain->pgtbl_cfg;
>   
>   	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>   		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> @@ -2220,16 +2251,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   
>   	switch (smmu_domain->stage) {
>   	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
>   		fmt = ARM_64_LPAE_S1;
>   		finalise_stage_fn = arm_smmu_domain_finalise_s1;
>   		break;
>   	case ARM_SMMU_DOMAIN_NESTED:
>   	case ARM_SMMU_DOMAIN_S2:
> -		ias = smmu->ias;
> -		oas = smmu->oas;
>   		fmt = ARM_64_LPAE_S2;
>   		finalise_stage_fn = arm_smmu_domain_finalise_s2;
>   		break;
> @@ -2237,24 +2263,19 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   		return -EINVAL;
>   	}
>   
> -	pgtbl_cfg = (struct io_pgtable_cfg) {
> -		.pgsize_bitmap	= smmu->pgsize_bitmap,
> -		.ias		= ias,
> -		.oas		= oas,
> -		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> -		.tlb		= &arm_smmu_flush_ops,
> -		.iommu_dev	= smmu->dev,
> -	};
> +	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, pgtbl_cfg);
> +	if (ret)
> +		return ret;
>   
> -	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
> +	pgtbl_ops = alloc_io_pgtable_ops(fmt, pgtbl_cfg, smmu_domain);
>   	if (!pgtbl_ops)
>   		return -ENOMEM;
>   
> -	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> -	domain->geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
> +	domain->pgsize_bitmap = pgtbl_cfg->pgsize_bitmap;
> +	domain->geometry.aperture_end = (1UL << pgtbl_cfg->ias) - 1;
>   	domain->geometry.force_aperture = true;
>   
> -	ret = finalise_stage_fn(smmu_domain, &pgtbl_cfg);
> +	ret = finalise_stage_fn(smmu_domain, pgtbl_cfg);
>   	if (ret < 0) {
>   		free_io_pgtable_ops(pgtbl_ops);
>   		return ret;
> @@ -2406,6 +2427,46 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>   	pci_disable_pasid(pdev);
>   }
>   
> +static int
> +arm_smmu_verify_domain_compatible(struct arm_smmu_device *smmu,
> +				  struct arm_smmu_domain *smmu_domain)
> +{
> +	struct io_pgtable_cfg pgtbl_cfg;
> +	int ret;
> +
> +	if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY)
> +		return 0;
> +
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
> +			return -EINVAL;
> +		if (smmu_domain->s2_cfg.vmid >> smmu->vmid_bits)
> +			return -EINVAL;
> +	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> +			return -EINVAL;
> +		if (smmu_domain->cd.asid >> smmu->asid_bits)
> +			return -EINVAL;
> +	}
> +
> +	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, &pgtbl_cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (smmu_domain->pgtbl_cfg.ias > pgtbl_cfg.ias ||
> +	    smmu_domain->pgtbl_cfg.oas > pgtbl_cfg.oas ||
> +	    /*
> +	     * The supported pgsize_bitmap must be a superset of the domain's
> +	     * pgsize_bitmap.
> +	     */
> +	    (smmu_domain->pgtbl_cfg.pgsize_bitmap ^ pgtbl_cfg.pgsize_bitmap) &
> +		    smmu_domain->pgtbl_cfg.pgsize_bitmap ||
> +	    smmu_domain->pgtbl_cfg.coherent_walk != pgtbl_cfg.coherent_walk)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static void arm_smmu_installed_smmus_remove_device(
>   	struct arm_smmu_domain *smmu_domain,
>   	struct arm_smmu_master *master)
> @@ -2449,6 +2510,10 @@ arm_smmu_installed_smmus_add_device(struct arm_smmu_domain *smmu_domain,
>   		}
>   	}
>   	if (!list_entry_found) {
> +		ret = arm_smmu_verify_domain_compatible(smmu, smmu_domain);
> +		if (ret)
> +			goto unlock;
> +
>   		installed_smmu = kzalloc(sizeof(*installed_smmu), GFP_KERNEL);
>   		if (!installed_smmu) {
>   			ret = -ENOMEM;
> 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 2ab23139c796e..143b287be2f8b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -9,6 +9,7 @@
>   #define _ARM_SMMU_V3_H
>   
>   #include <linux/bitfield.h>
> +#include <linux/io-pgtable.h>
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/mmzone.h>
> @@ -729,6 +730,7 @@ struct arm_smmu_domain {
>   	struct mutex			init_mutex; /* Protects smmu pointer */
>   
>   	struct io_pgtable_ops		*pgtbl_ops;
> +	struct io_pgtable_cfg		pgtbl_cfg;
>   	atomic_t			nr_ats_masters;
>   
>   	enum arm_smmu_domain_stage	stage;
  

On Fri, Aug 18, 2023 at 3:34 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2023-08-17 19:16, Michael Shavit wrote:
> > Add a new arm_smmu_installed_smmu class to aggregate masters belonging
> > to the same SMMU that a domain is attached to.
> > Update usages of the domain->devices list to first iterate over this
> > parent installed_smmus list.
> >
> > This allows functions that batch commands to an SMMU to first iterate
> > over the list of installed SMMUs before preparing the batched command
> > from the set of attached masters.
>
> I get the feeling that any purpose this serves could be achieved far
> more simply by just keeping the lists sorted by SMMU instance.

I initially tried just that, but then you need more sophisticated code
when iterating over the list to correctly batch operations by SMMU,
which is the more common operation.
That first hunk is very unfortunate though... Let me revive that
version and send it out so we can better compare.