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 <sk...@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.suthikulpa...@amd.com>
Tested-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>

Thanks,
Suravee
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to