Messages in this thread | | | From | Suravee Suthikulpanit <> | Subject | Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init | Date | Fri, 24 Jan 2020 13:43:37 +0700 |
| |
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
| |