Re: [PATCH v2 10/16] iommu/amd: Modify logic for checking GT and PPR features

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

On 7/28/2023 7:54 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:03AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> In order to support v2 page table, IOMMU driver need to check if the
>> hardware can support Guest Translation (GT) and Peripheral Page Requet
>> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
>> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
>> features. There variables area redundant since we could simply just check
>> the global EFR mask.
>>
>> Therefore, replace it with a helper function with appropriate name.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h       | 11 +++++++++++
>>  drivers/iommu/amd/amd_iommu_types.h |  9 ++++-----
>>  drivers/iommu/amd/init.c            | 14 +-------------
>>  drivers/iommu/amd/iommu.c           |  2 +-
>>  4 files changed, 17 insertions(+), 19 deletions(-)
> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index a0b0deb6fbcb..1f707944b23f 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -392,7 +392,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>>  	 */
>>  	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
>>  	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
>> -		dev_data->iommu_v2 = iommu->is_iommu_v2;
>> +		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
> 
> This doesn't make alot of sense to me, we should not have global
> functions and data in drivers. In this case I would expect the driver
> to consult the amd_iommu linked to the struct device it is working on
> to determine the capability.
> 
> Eg just directly test:
> 
> 	iommu_feature(iommu, FEATURE_GT) && iommu_feature(iommu, FEATURE_PPR)
> 
> And remove all the other copies of the iommu_v2

Yes. Eventually we are removing iommu_v2 variable from both iommu and device
structures.

> 
> And arguably this can (eventually) be defered to an attach that
> requires the gcr3 table.

Yes. Later part of the SVA series does this. Basically init discovers the device
capabilities, attach/detach_device() takes care of enabling/disabling features.
Finally def_domain_type() decides best page table mode for the given device.

-Vasant

> 
> Also, if the gcr3 table is supported or not should be entirely up to
> the HW, policy inputs like "iommu_default_passthrough" and
> "force_isolation" are nonsensical here..
> 
> Looking in the history this looks like a bodge to cover up the domain
> type mismatch during allocation. We need to get to a point where the
> device is known during domain allocation so the driver can provide a
> v2 page table if the device could possibly use PASID and remove all
> this hackery.
> 
> Jason