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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Tested-by: Suravee Suthikulpanit <[email protected]>
Thanks,
Suravee
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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 <[email protected]>
> Tested-by: Suravee Suthikulpanit <[email protected]>
>
Thanks for testing it.
-- Shuah