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

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 3/5/2024 6:02 AM, Jason Gunthorpe wrote:
> 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.

We are setting up handler right after enabling capability for both IDENTIFY and
v2 domain as both can support SVA mode.

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

Yes. If device supports ATS then its enabled 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.

Our V1 page table is not compatible with PASID. Hence currently its done in
attach_device path.

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

Correct. I will revisit some of these code paths later.

-Vasant

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