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

[Shuah Khan <skhan@linuxfoundation.org>]

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