Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init

[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@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
> Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

Thanks for testing it.

-- Shuah