Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during
 init
To:     Shuah Khan <skhan@linuxfoundation.org>, joro@8bytes.org
Cc:     iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        "Grimm, Jon" <jon.grimm@amd.com>
References: <20200123223214.2566-1-skhan@linuxfoundation.org>
Message-ID: <096ee758-a372-4caa-c082-e1e8cff3f033@amd.com>
Date:   Fri, 24 Jan 2020 13:43:37 +0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
In-Reply-To: <20200123223214.2566-1-skhan@linuxfoundation.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Received-SPF: None (protection.outlook.com: amd.com does not designate
 permitted sender hosts)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1a0621e4-c678-4595-45c0-08d7a098da8f
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2020 06:44:26.1348
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AOLM+e+lqGZrKwvtQRh6tldA0/zhH7dM32z0WunhvGwhvB3Ls4oHIu60cp16ihviAl+qn2nobUpmRY3ADAhFMg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB2651
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/24/20 5:32 AM, Shuah Khan wrote:
> init_iommu_perf_ctr() clobbers the register when it checks write access
> to IOMMU perf counters and fails to restore when they are writable.
> 
> Add save and restore to fix it.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> Changes since v1:
> -- Fix bug in sucessful return path. Add a return instead of
>     fall through to pc_false error case
> 
>   drivers/iommu/amd_iommu_init.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 568c52317757..483f7bc379fa 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1655,27 +1655,39 @@ static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
>   static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>   {
>   	struct pci_dev *pdev = iommu->dev;
> -	u64 val = 0xabcd, val2 = 0;
> +	u64 val = 0xabcd, val2 = 0, save_reg = 0;
>   
>   	if (!iommu_feature(iommu, FEATURE_PC))
>   		return;
>   
>   	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))
> +		goto pc_false;
> +
>   	/* Check if the performance counters can be written to */
>   	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)) {
> -		pci_err(pdev, "Unable to write to IOMMU perf counter.\n");
> -		amd_iommu_pc_present = false;
> -		return;
> -	}
> +	    (val != val2))
> +		goto pc_false;
> +
> +	/* 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");
>   
>   	val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
>   	iommu->max_banks = (u8) ((val >> 12) & 0x3f);
>   	iommu->max_counters = (u8) ((val >> 7) & 0xf);
> +
> +	return;
> +

Good catch. Sorry, I missed this part as well :(

> +pc_false:
> +	pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
> +	amd_iommu_pc_present = false;
> +	return;
>   }
>   
>   static ssize_t amd_iommu_show_cap(struct device *dev,
> 

As for your question in v1:

 > I see 2 banks and 4 counters on my system. Is it sufficient to check
 > the first bank and first counter? In other words, if the first one
 > isn't writable, are all counters non-writable?

We currently assume all counters have the same write-ability. So, it should be sufficient
to just check the first one.

 > Should we read the config first and then, try to see if any of the
 > counters are writable? I have a patch that does that, I can send it
 > out for review.

Which config are you referring to? Not sure what you mean.

By the way, here is the output from booting the system with this patch (+ debug messages).

[   14.408834] pci 0000:60:00.2: AMD-Vi: IOMMU performance counters supported
[   14.416526] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[   14.429602] pci 0000:40:00.2: AMD-Vi: IOMMU performance counters supported
[   14.437275] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[   14.450320] pci 0000:20:00.2: AMD-Vi: IOMMU performance counters supported
[   14.457991] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[   14.471049] pci 0000:00:00.2: AMD-Vi: IOMMU performance counters supported
[   14.478722] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1

Also, here is the perf amd_iommu testing.

# perf stat -e 'amd_iommu_0/cmd_processed/,\
		amd_iommu_1/cmd_processed/,\
		amd_iommu_2/cmd_processed/,\
		amd_iommu_3/cmd_processed/'

  Performance counter stats for 'system wide':

                204      amd_iommu_0/cmd_processed/
                  0      amd_iommu_1/cmd_processed/
                472      amd_iommu_2/cmd_processed/
                  2      amd_iommu_3/cmd_processed/

       10.198257728 seconds time elapsed

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee