Re: [PATCH v6 08/15] iommu/amd: Enable PCI features based on attached domain capability

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Feb 09, 2024 at 11:29:23AM +0000, Vasant Hegde wrote:
> Commit eda8c2860ab ("iommu/amd: Enable device ATS/PASID/PRI capabilities
> independently") changed the way it enables device capability while
> attaching devices. I missed to account the attached domain capability.
> Meaning if domain is not capable of handling PASID/PRI (ex: paging
> domain with v1 page table) then enabling device feature is not required.
> 
> This patch enables PASID/PRI only if domain is capable of handling SVA.
> Also move pci feature enablement to do_attach() function so that we make
> SVA capability in one place. Finally make PRI enable/disable functions as
> static functions.

> @@ -2036,6 +2038,7 @@ static int do_attach(struct iommu_dev_data *dev_data,
>  		     struct protection_domain *domain)
>  {
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
> +	struct pci_dev *pdev;
>  	int ret = 0;
>  
>  	/* Update data structures */
> @@ -2050,10 +2053,16 @@ static int do_attach(struct iommu_dev_data *dev_data,
>  	domain->dev_iommu[iommu->index] += 1;
>  	domain->dev_cnt                 += 1;
>  
> +	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
>  	if (pdom_is_sva_capable(domain)) {
>  		ret = init_gcr3_table(dev_data, domain);
>  		if (ret)
>  			return ret;
> +
> +		if (pdev)
> +			pdev_enable_caps(pdev);

But here we are turning PRI on for an IDENTITY domain and for a v2
domain that doesn't have a PRI handler. This is not technically what
we want to see, the PRI capability at the device should only be turned
on when a PRI handler is available in the SW side. Otherwise PRI
should be off and non-present ATS responses should fail in the device.

ATS is not like that, ATS should be enabled whenever the system iommu
can support it. For AMD I guess that is always?

Also, PASID enablement looks like the wrong spot too, it should be
done in the probe_device() callback and stay enabled. The bit is set
if the IOMMU HW can decode the PASID TLP. Ie don't enable it on old
IOMMU HW that can't understand PASID.

However all of this is basically pre-existing problems at this point
and this patch is improving things as they are so

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason