Re: [PATCH v6 17/24] iommu/amd: Program nested DTE and DomID map on attach

[Jason Gunthorpe]

> [ ... 150 lines skipped ... ]
> @@ -251,10 +268,23 @@ static int nested_attach_device(struct iommu_domain *dom, struct device *dev,
>  
>  	mutex_lock(&dev_data->mutex);
>  
> -	set_dte_nested(iommu, dom, dev_data, &new);
> +	ret = set_dte_nested(iommu, dom, dev_data, &new);
> +	if (ret)
> +		goto out_err;
>  
> +	/*
> +	 * Program gdom_id -> hdom_id before the DTE is live with
> +	 * DTE_VIOMMU_EN. Hold vfctrl_lock across both so a concurrent
> +	 * last-ref free cannot restore the idle map in between.
> +	 */
> +	mutex_lock(&ndom->viommu->vfctrl_lock);
> +	amd_viommu_domain_id_update_locked(ndom->viommu,
> +					   ndom->gdom_info->hdom_id,
> +					   ndom->gdom_id);
>  	amd_iommu_update_dte(iommu, dev_data, &new);
> +	mutex_unlock(&ndom->viommu->vfctrl_lock);

I didn't find an explanation for why this is asymetric, or what the
overall lifecycle of the mapping is.

I would have expected the maintaince of the HW table to be aligned to
the gdom_info lifecycle. Establish a gdom/hdom mapping when the
gdom_info is allocated, free it when it is freed.

Instead we set it on the first device to attach and then free it when
the first sharing nested domain is freed?

That doesn't seem right?

> [ ... 24 lines skipped ... ]
> @@ -282,14 +320,34 @@ static void nested_domain_free(struct iommu_domain *dom)
>  			    ndom->gdom_info, NULL, GFP_ATOMIC);
>  
>  	xa_unlock_irqrestore(&aviommu->gdomid_array, irqflags);
> -	if (WARN_ON(!curr || xa_err(curr)))
> +	if (WARN_ON(!curr || xa_err(curr))) {
> +		mutex_unlock(&aviommu->vfctrl_lock);
>  		return;
> +	}
>  
> -	/* success */
>  	pr_debug("%s: Free gdom_id=%#x, hdom_id=%#x\n",
> -		__func__, ndom->gdom_id, curr->hdom_id);
> +		 __func__, ndom->gdom_id, curr->hdom_id);
>  
> -	amd_iommu_pdom_id_free(ndom->gdom_info->hdom_id);
> +	/*
> +	 * Restore the idle DomID map before releasing hdom_id. Every
> +	 * slot stays V=1; unknown mapping raises an event. Idle is the
> +	 * nest parent CONTROL1 map, same as vIOMMU init prefill.
> +	 * amd_iommu_reset_vmmio() only resets guest MMIO.
> +	 *
> +	 * Guest INVALIDATE_IOMMU_ALL walks the HW table, not
> +	 * gdomid_array. The pdom_ids IDA is global, so a stale V=1
> +	 * entry can invalidate a recycled hdom_id that now belongs to
> +	 * another domain.
> +	 *
> +	 * Detach does not clear this slot: hdom_id stays allocated
> +	 * until this last free, so a lingering gdom_id -> hdom_id
> +	 * map cannot point at a recycled ID.
> +	 */
> +	amd_viommu_domain_id_update_locked(aviommu, aviommu->parent->id,
> +					   ndom->gdom_id);
> +	mutex_unlock(&aviommu->vfctrl_lock);
> +
> +	amd_iommu_pdom_id_free(curr->hdom_id);

What is the protocol for flushing the hdom_id from the IOTLB?

I can't remember how AMD works, but generally it is a good idea for
IDs in the IDR to be known to be cleared out from the IOTLB so they
are always clean. I don't see something like that here. Is it OK?

> [ ... 49 lines skipped ... ]
> +void amd_viommu_domain_id_update_locked(struct amd_iommu_viommu *aviommu,
> +					u16 hdom_id, u16 gdom_id)
> +{
> +	struct amd_iommu *iommu =
> +		container_of(aviommu->core.iommu_dev, struct amd_iommu, iommu);
> +
> +	lockdep_assert_held(&aviommu->vfctrl_lock);
> +	domain_id_update_unlocked(iommu, aviommu->gid, hdom_id, gdom_id);

A _locked function calls an _unlocked function? That's confusing..

-- 
Jason