Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

From: Jason Gunthorpe <jgg@nvidia.com>
To: Michael Shavit <mshavit@google.com>
Cc: iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, will@kernel.org,
	robin.murphy@arm.com, nicolinc@nvidia.com,
	jean-philippe@linaro.org, tina.zhang@intel.com,
	Joerg Roedel <joro@8bytes.org>, Kevin Tian <kevin.tian@intel.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Mark Brown <broonie@kernel.org>, Tomas Krcka <krckatom@amazon.de>,
	Yicong Yang <yangyicong@hisilicon.com>
Subject: Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond
Date: Tue, 5 Sep 2023 09:42:20 -0300	[thread overview]
Message-ID: <ZPciLKG2Gi/Biovp@nvidia.com> (raw)
In-Reply-To: <20230905194849.v1.3.I211f2ab0ee241f53cdfbc3a8a573f14b8a46fb26@changeid>

On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote:
> Create a new iommu_domain subclass for SVA iommu domains to hold the
> data previously stored in the dynamically allocated arm_smmu_bond. Add a
> simple count of attached SVA domains to arm_smmu_master to replace the
> list of bonds.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
>  3 files changed, 26 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 9fb6907c5e7d4..0342c0f35d55a 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
>  
>  #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
>  
> -struct arm_smmu_bond {
> -	struct mm_struct		*mm;
> +struct arm_smmu_sva_domain {
> +	struct iommu_domain		iommu_domain;
>  	struct arm_smmu_mmu_notifier	*smmu_mn;
> -	struct list_head		list;
>  };
>  
> -#define sva_to_bond(handle) \
> -	container_of(handle, struct arm_smmu_bond, sva)
> +#define to_sva_domain(domain) \
> +	container_of(domain, struct arm_smmu_sva_domain, iommu_domain)

I'm not sure about this? This seems like a strange direction

The SVA domain and a UNMANAGED/PAGING domain should be basically the
same thing. Making a sva_domain a completely different type looks like
it would stand in the way of that?
> @@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
>  
>  struct iommu_domain *arm_smmu_sva_domain_alloc(void)
>  {
> -	struct iommu_domain *domain;
> +	struct arm_smmu_sva_domain *sva_domain;
>  
> -	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> -	if (!domain)
> +	sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
> +	if (!sva_domain)
>  		return NULL;
> -	domain->ops = &arm_smmu_sva_domain_ops;
> -
> -	return domain;
> +	sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;

arm_smmu_sva_domain_free() should container_of before freeing, but
gross to assume the iommu_domain is the first member.

Jason