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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Aug 07, 2023 at 10:14:23PM +0530, Vasant Hegde wrote:
> Hi Jason,
> 
> 
> On 8/4/2023 6:49 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 04, 2023 at 06:42:10AM +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/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> >> index a5a350ee36fe..0605f02fa711 100644
> >> --- a/drivers/iommu/amd/amd_iommu.h
> >> +++ b/drivers/iommu/amd/amd_iommu.h
> >> @@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
> >>  	return !!(iommu->features & mask);
> >>  }
> >>  
> >> +static inline bool check_feature_on_all_iommus(u64 mask)
> >> +{
> >> +	return !!(amd_iommu_efr & mask);
> >> +}
> >> +
> >> +static inline bool amd_iommu_gt_ppr_supported(void)
> >> +{
> >> +	return (check_feature_on_all_iommus(FEATURE_GT) &&
> >> +		check_feature_on_all_iommus(FEATURE_PPR));
> >> +}
> >> +
> > 
> > I'm still against adding more globals, the iommu struct was available,
> > just use it directly in this patch.
> 
> Sorry. I missed to append the reason after re-generating patch series.
> 
> We want to make sure features supported by all IOMMUs are consistent.
> Hence we introduced this function. This function will be used in
> subsequent series (ex:  before enabling IOMMU PPR feature, etc).

That isn't really the right direction. iommus are per-instance things,
each instance should attend to its own stuff. Globals should be avoided.

> Also there are features like SNP which has requirement that all
> IOMMUs supports features before enabling it. So moving all checks to
> check_feature_on_all_iommu() makes it easy.

It is confusing why this would be necessary, but even if it is, it
should be limitd to SNP with a big comment explaining why SNP is
broken.

Jason