Re: [PATCH 1/2] Revert "iommu/amd: Fix performance counter initialization"

[Shuah Khan]

On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>
> This reverts commit 6778ff5b21bd8e78c8bd547fd66437cf2657fd9b.
>
> The original commit tries to address an issue, where PMC power-gating
> causing the IOMMU PMC pre-init test to fail on certain desktop/mobile
> platforms where the power-gating is normally enabled.
>
> There have been several reports that the workaround still does not
> guarantee to work, and can add up to 100 ms (on the worst case)
> to the boot process on certain platforms such as the MSI B350M MORTAR
> with AMD Ryzen 3 2200G.
>
> Therefore, revert this commit as a prelude to removing the pre-init
> test.
>
> Link: https://lore.kernel.org/linux-iommu/alpine.LNX.3.20.13.2006030935570.3181@xxxxxxxxxxxxxxxxxxxxxxx/
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
> Cc: Tj (Elloe Linux) <ml.linux@elloe.vision>
> Cc: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> Cc: Alexander Monakov <amonakov@xxxxxxxxx>
> Cc: David Coe <david.coe@xxxxxxxxxx>
> Signed-off-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
> Note: I have revised the commit message to add more detail
>        and remove uncessary information.
>
>   drivers/iommu/amd/init.c | 45 ++++++++++------------------------------
>   1 file changed, 11 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 321f5906e6ed..648cdfd03074 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -12,7 +12,6 @@
>   #include <linux/acpi.h>
>   #include <linux/list.h>
>   #include <linux/bitmap.h>
> -#include <linux/delay.h>
>   #include <linux/slab.h>
>   #include <linux/syscore_ops.h>
>   #include <linux/interrupt.h>
> @@ -257,8 +256,6 @@ static enum iommu_init_state init_state = IOMMU_START_STATE;
>   static int amd_iommu_enable_interrupts(void);
>   static int __init iommu_go_to_state(enum iommu_init_state state);
>   static void init_device_table_dma(void);
> -static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
> -				u8 fxn, u64 *value, bool is_write);
>   
>   static bool amd_iommu_pre_enabled = true;
>   
> @@ -1717,11 +1714,13 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>   	return 0;
>   }
>   
> -static void __init init_iommu_perf_ctr(struct amd_iommu *iommu)
> +static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
> +				u8 fxn, u64 *value, bool is_write);
> +
> +static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>   {
> -	int retry;
>   	struct pci_dev *pdev = iommu->dev;
> -	u64 val = 0xabcd, val2 = 0, save_reg, save_src;
> +	u64 val = 0xabcd, val2 = 0, save_reg = 0;
>   
>   	if (!iommu_feature(iommu, FEATURE_PC))
>   		return;
> @@ -1729,39 +1728,17 @@ static void __init init_iommu_perf_ctr(struct amd_iommu *iommu)
>   	amd_iommu_pc_present = true;
>   
>   	/* save the value to restore, if writable */
> -	if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false) ||
> -	    iommu_pc_get_set_reg(iommu, 0, 0, 8, &save_src, false))
> -		goto pc_false;
> -
> -	/*
> -	 * Disable power gating by programing the performance counter
> -	 * source to 20 (i.e. counts the reads and writes from/to IOMMU
> -	 * Reserved Register [MMIO Offset 1FF8h] that are ignored.),
> -	 * which never get incremented during this init phase.
> -	 * (Note: The event is also deprecated.)
> -	 */
> -	val = 20;
> -	if (iommu_pc_get_set_reg(iommu, 0, 0, 8, &val, true))
> +	if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false))
>   		goto pc_false;
>   
>   	/* Check if the performance counters can be written to */
> -	val = 0xabcd;
> -	for (retry = 5; retry; retry--) {
> -		if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true) ||
> -		    iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false) ||
> -		    val2)
> -			break;
> -
> -		/* Wait about 20 msec for power gating to disable and retry. */
> -		msleep(20);
> -	}
> -
> -	/* restore */
> -	if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true) ||
> -	    iommu_pc_get_set_reg(iommu, 0, 0, 8, &save_src, true))
> +	if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
> +	    (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
> +	    (val != val2))

Probably don't need parentheses around 'val != val2'

>   		goto pc_false;
>   
> -	if (val != val2)
> +	/* restore */
> +	if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true))
>   		goto pc_false;
>   
>   	pci_info(pdev, "IOMMU performance counters supported\n");

thanks,
-- Shuah