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

[Jason Gunthorpe <jgg@ziepe.ca>]

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

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

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