Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

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, baolu.lu@linux.intel.com,
	will@kernel.org, jean-philippe@linaro.org, robin.murphy@arm.com,
	nicolinc@nvidia.com
Subject: Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids
Date: Thu, 3 Aug 2023 12:42:07 -0300	[thread overview]
Message-ID: <ZMvKzwx2IhgXO+F8@nvidia.com> (raw)
In-Reply-To: <20230803181225.v5.2.I8db07b9eaef3bd5ef9bfc5c8c6d44768a4d95293@changeid>

On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> The arm-smmu-v3 driver keeps track of all masters that a domain is
> attached to so that it can re-write their STEs when the domain's ASID is
> upated by SVA.

Wah?

A domain's ASID shouldn't change, why does it change for SVA? Doesn't
SVA use CDTE's only? Why would it ever change a STE? I'm confused what
you are trying to explain here.

> +/*
> + * When ssid is 0, update all the CD entries that this domain is attached to.
> + * When ssid is non-zero, write the CD into all the masters where this domain is
> + * the primary domain, with the provided SSID. This is used because SVA still
> + * piggybacks over the primary domain.
> + */

What is a "primary domain"? Why can't we fix SVA first so it doesn't
have this weird "piggybacks" or:

> +/*
> + * If ssid is non-zero, issue atc invalidations with the given ssid instead of
> + * the one the domain is attached to. This is used by SVA since it's pasid
> + * attachments aren't recorded in smmu_domain yet.
> + */

fails to be recorded?

This patch is not making sense to me, the goal in the commit message
seems logical, but why is tracking CD entries introducing this concept
of a primary domain and doing special stuff for SSID=0?

> 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 e76452e735a04..66a492cafe2e8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -689,11 +689,19 @@ struct arm_smmu_stream {
>  	struct rb_node			node;
>  };
>  
> +/* List of {masters, ssid} that a domain is attached to */
> +struct arm_smmu_attached_domain {
> +	struct list_head	list;

I like to call this something like

 attached_ssids_item

To help understand where the list head is and that this is a list
element.

> +	struct arm_smmu_domain  *domain;
> +	struct arm_smmu_master  *master;
> +	int			ssid;
> +};
> +
>  /* SMMU private data for each master */
>  struct arm_smmu_master {
>  	struct arm_smmu_device		*smmu;
>  	struct device			*dev;
> -	struct arm_smmu_domain		*domain;
> +	struct arm_smmu_attached_domain	non_pasid_domain;

Probably should consistently use the word ssid not pasid in driver
internals. 

The smmu spec talks about the substream ID being optional and the
behavior is controleld by the STE.S1DSS (Default substream)

So maybe non_subtream_domain ?

> @@ -730,8 +738,12 @@ struct arm_smmu_domain {
>  
>  	struct iommu_domain		domain;
>  
> -	struct list_head		devices;
> -	spinlock_t			devices_lock;
> +	/*
> +	 * List of arm_smmu_attached_domain nodes used to track all the
> +	 * {master, ssid} pairs that this domain is attached to.
> +	 */
> +	struct list_head		attached_ssids;
> +	spinlock_t			attached_ssids_lock;

So ssid = 0 means that the list entry == master->non_pasid_domain ?

It would be clearer to just test that directly if that is what needs
to be determined.

At least these struct changes seem aligned with the commit message :)

Jason

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