Re: [PATCH 3/5] iommu/amd: Introduce Disable IRTE Caching Support

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

Hi Suravee,

A couple of additional comments below:

On 5/9/2023 7:16 AM, Suravee Suthikulpanit wrote:
> An Interrupt Remapping Table (IRT) stores interrupt remapping configuration
> for each device. In a normal operation, the AMD IOMMU caches the table
> to optimize subsequent data accesses. This requires the IOMMU driver to
> invalidate IRT whenever it updates the table. The invalidation process
> includes issuing an INVALIDATE_INTERRUPT_TABLE command following by
> a COMPLETION_WAIT command.
>
> However, there are cases in which the IRT is updated at a high rate.
> For example, for IOMMU AVIC, the IRTE[IsRun] bit is updated on every
> vcpu scheduling (i.e. amd_iommu_update_ga()). On system with large
> amount of vcpus and VFIO PCI pass-through devices, the invalidation
> process could potentially become a performance bottleneck.
>
> Introducing a new kernel boot option:
>
>      amd_iommu=irtcachedis
>
> which disables IRTE caching by setting the IRTCachedis bit in each IOMMU
> Control register, and bypass the IRT invalidation process.
>
> Co-developed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> [Awaiting sign-off-by Alejandro]
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   .../admin-guide/kernel-parameters.txt         |  1 +
>   drivers/iommu/amd/amd_iommu_types.h           |  4 +++
>   drivers/iommu/amd/init.c                      | 25 +++++++++++++++++++
>   3 files changed, 30 insertions(+)
[snip]
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index fd487c33b28a..01d131e75de4 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -160,6 +160,7 @@ static int amd_iommu_xt_mode = IRQ_REMAP_XAPIC_MODE;
>   static bool amd_iommu_detected;
>   static bool amd_iommu_disabled __initdata;
>   static bool amd_iommu_force_enable __initdata;
> +static bool amd_iommu_irtcachedis __initdata;
Lets drop the __initdata attribute above, since amd_iommu_irtcachedis is 
used by early_enable_iommus(), which is in .text (causes modpost warning).

[snip]
>   
> +static void iommu_enable_irtcachedis(struct amd_iommu *iommu)
> +{
> +	u64 ctrl;
> +
> +	if (amd_iommu_irtcachedis) {
> +		/*
> +		 * Note:
> +		 * The support for IRTCacheDis feature is dertermined by
> +		 * checking if the bit is writable.
> +		 */
> +		iommu_feature_enable(iommu, CONTROL_IRTCACHEDIS);
> +		ctrl = readq(iommu->mmio_base +  MMIO_CONTROL_OFFSET);
> +		ctrl &= (1ULL << CONTROL_IRTCACHEDIS);
> +		if (ctrl)
> +			iommu->irtcachedis_enabled = true;
> +		pr_info("iommu%d (%#06x) : IRT cache is %s\n",
> +			iommu->index, iommu->devid,
> +			iommu->irtcachedis_enabled ? "disabled" : "enabled");
> +	}
> +}
> +
>   static void early_enable_iommu(struct amd_iommu *iommu)
>   {
>   	iommu_disable(iommu);
> @@ -2710,6 +2732,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
>   	iommu_set_exclusion_range(iommu);
>   	iommu_enable_ga(iommu);
>   	iommu_enable_xt(iommu);
> +	iommu_enable_irtcachedis(iommu);
>   	iommu_enable(iommu);
>   	iommu_flush_all_caches(iommu);
>   }
I need to understand better the code flow around kdump, and it is not 
clear from my reading of the spec that this is required, but shouldn't 
iommu_enable_irtcachedis() also be called in the else{} block of 
early_enable_iommus()?

Thank you,
Alejandro