* [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
@ 2020-01-23 22:32 Shuah Khan
2020-01-24 6:43 ` Suravee Suthikulpanit
2020-01-24 14:29 ` Joerg Roedel
0 siblings, 2 replies; 4+ messages in thread
From: Shuah Khan @ 2020-01-23 22:32 UTC (permalink / raw)
To: joro, suravee.suthikulpanit; +Cc: iommu, linux-kernel, Shuah Khan
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;
+
+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,
--
2.20.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
2020-01-23 22:32 [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init Shuah Khan
@ 2020-01-24 6:43 ` Suravee Suthikulpanit
2020-01-24 17:59 ` Shuah Khan
2020-01-24 14:29 ` Joerg Roedel
1 sibling, 1 reply; 4+ messages in thread
From: Suravee Suthikulpanit @ 2020-01-24 6:43 UTC (permalink / raw)
To: Shuah Khan, joro; +Cc: iommu, Grimm, Jon, linux-kernel
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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
2020-01-23 22:32 [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init Shuah Khan
2020-01-24 6:43 ` Suravee Suthikulpanit
@ 2020-01-24 14:29 ` Joerg Roedel
1 sibling, 0 replies; 4+ messages in thread
From: Joerg Roedel @ 2020-01-24 14:29 UTC (permalink / raw)
To: Shuah Khan; +Cc: iommu, linux-kernel
On Thu, Jan 23, 2020 at 03:32:14PM -0700, 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(-)
Applied for v5.5, thanks.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
2020-01-24 6:43 ` Suravee Suthikulpanit
@ 2020-01-24 17:59 ` Shuah Khan
0 siblings, 0 replies; 4+ messages in thread
From: Shuah Khan @ 2020-01-24 17:59 UTC (permalink / raw)
To: Suravee Suthikulpanit, joro
Cc: iommu, Grimm, Jon, linux-kernel, skh >> Shuah Khan
On 1/23/20 11:43 PM, Suravee Suthikulpanit wrote:
>
>
> 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.
I mean reading MMIO_CNTR_CONF_OFFSET to get max banks and counters.
Also what is the reason to check writable?
I tried a couplf og things on my
AMD Ryzen 5 PRO 2400GE w/ Radeon Vega Graphics
I changed the logic to read config to get max banks and counters
before checking if counters are writable and tried writing to all.
The result is the same and all of them aren't writable. However,
when disable the writable check and assume they are, I can run
perf stat -e 'amd_iommu_0 on all events and get data.
perf stat -e 'amd_iommu_0/cmd_processed/' sleep 10
Performance counter stats for 'system wide':
56 amd_iommu_0/cmd_processed/
10.001525171 seconds time elapsed
perf stat -a -e amd_iommu/mem_trans_total/ sleep 10
Performance counter stats for 'system wide':
2,696 amd_iommu/mem_trans_total/
10.001465115 seconds time elapsed
I tried all possible events listed under amd_iommu_0 and I can get
data on all of them. No problems in dmesg.
Is this inline with what you expect? I am curious what this write
tell us and can we enable read only mode on these counters?
>
> 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 for testing it.
-- Shuah
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-24 17:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 22:32 [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init Shuah Khan
2020-01-24 6:43 ` Suravee Suthikulpanit
2020-01-24 17:59 ` Shuah Khan
2020-01-24 14:29 ` Joerg Roedel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).