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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Aug 10, 2023 at 01:31:41PM -0700, Suthikulpanit, Suravee wrote:
> 
> Actually, it is not just for the SNP.
> 
> Please let me further clarify the existence of the global amd_iommu_efr and
> amd_iommu_efr2 variables.
> 
> EFR and EFR2 are provided in two places:
>  1. The IVHD block in the ACPI IVRS table.
>  2. The register offset 0030h (EFR) and 01A0h (EFR2).
> where information in the IVHD block supersedes the MMIO registers.
> 
> The EFR/EFR2 information is needed early in the initialization process prior
> to amd_iomu_init_pci() (where we can start accessing the MMIO registers).
> So, current code ignore information in the MMIO registers (only check and
> warn in case of the IVHD and MMIO are mismatch).
> 
> Currently, IOMMU driver assumes capabilities on all IOMMU instances to be
> homogeneous. Therefore, during early_amd_iommu_init(), the driver probes all
> IVHD blocks and do sanity check to make sure that only features common among
> all IOMMU instances are supported. This is tracked in the global
> amd_iommu_efr / amd_iommu_efr2.

Assuming homogeneous is sort of the point - things should be localized
to their instances so you don't need to make this assumption, that
means minimizing the places that touch the global version

> Therefore, the helper function check_feature_on_all_iommus() uses the
> effective amd_iommu_efr/amd_iommu_efr2 instead of iommu->features. In facts,
> we should remove the code which directly uses the iommu->features,
> iommu->features2, and the iommu_feature() helper function to simply and make
> the code less confusing.

But if you do this I would not object - I just don't think it is good
driver design *for the iommu*

However..

Looking at it I only see two places that need the global 'all iommus'
check - one is amd_iommu_snp_enable() which is never called (seriously
stop adding dead code to support your out of tree patch sets!!!)

And the other is enable_iommus_vapic() which looks like it has the
iommus probed already.

So really this is 'we need it global for some SNP thing that isn't
even in the kernel' which is a really, really, bad justification.
 
Jason