Re: [PATCH 09/11] iommu/amd: Introduce helper functions for AMD IOMMU v2 driver

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Aug 08, 2023 at 10:02:30AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Introducing new functions to init and uninit v2 domain. Also setup /
> clear GCR3 table. These functions will make necessary changes to
> existing domain to switch to SVA mode. 

Huh? "necessary changes to existing domains" ? That shouldn't be
something a driver ever does.

> +/*******************************
> + * V2API-related helper functions
> + * Note: Separate these out for now. It will be removed when
> + *       deprecate the AMD IOMMU v2 API support.
> + */
> +int amd_iommu_v2api_domain_init(struct protection_domain *pdom)
> +{
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&pdom->lock, flags);
> +
> +	/* Only allow v2API support on pass-through domain. */
> +	if (pdom->pd_mode != PD_MODE_PT) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	pdom->pd_mode = PD_MODE_V2;

Wah?? You change a PT domain to a V2 domain? That seems totally
nonsensical.

This is continuing the wrong direction of the old code.

When you first create the GCR3 table you have to inspect which domain
is attached to the RID and setup the DTE/GCR3 accordingly.

You don't *change* the domain that is attached to the RID (other
things might be using it!) You just note that it is IDENTITY and do
the right things.

This should all be part of generic code that is working with the GCR3
object, not here in the v2api section.

Keep in mind that the API flow is to get a 'ref' on the GCR3 (eg
because the caller wants to put a domain in it) and a pairing 'unref'.

eg if it was already a v2 PT when you want to attach a PASID then
great, the device already has a gcr3 and so it just keeps
going. Having a PASID will block the GCR3 from being freed and it will
block V1 domains from being attached to the RID.

When the GCR3 becomes fully unrefed then all the PASIDs must be
removed, the RID must be an IDENTIY and then the driver can free the
GCR3 and reconfigure the DTE back to IDENTITY. Otherwise the DTE
points to the kernel owned GCR3.

Jason