Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] iommu/amd: Remove performance counter
 pre-initialization test
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org
Cc:     joro@8bytes.org, will@kernel.org, jsnitsel@redhat.com,
        pmenzel@molgen.mpg.de, Jon.Grimm@amd.com,
        Tj <ml.linux@elloe.vision>,
        Alexander Monakov <amonakov@ispras.ru>,
        David Coe <david.coe@live.co.uk>,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20210409085848.3908-1-suravee.suthikulpanit@amd.com>
 <20210409085848.3908-3-suravee.suthikulpanit@amd.com>
 <6d53ded8-9ab8-7205-f17e-d0c315b770bd@linuxfoundation.org>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <4f64e9d4-dfc5-3dec-32c9-06bfbfeafd6b@linuxfoundation.org>
Date:   Fri, 9 Apr 2021 11:10:29 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <6d53ded8-9ab8-7205-f17e-d0c315b770bd@linuxfoundation.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 4/9/21 10:37 AM, Shuah Khan wrote:
> On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
>> In early AMD desktop/mobile platforms (during 2013), when the IOMMU
>> Performance Counter (PMC) support was first introduced in
>> commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
>> resource management"), there was a HW bug where the counters could not
>> be accessed. The result was reading of the counter always return zero.
>>
>> At the time, the suggested workaround was to add a test logic prior
>> to initializing the PMC feature to check if the counters can be 
>> programmed
>> and read back the same value. This has been working fine until the more
>> recent desktop/mobile platforms start enabling power gating for the PMC,
>> which prevents access to the counters. This results in the PMC support
>> being disabled unnecesarily.
> 
> unnecessarily
> 
>>
>> Unfortunatly, there is no documentation of since which generation
> 
> Unfortunately,
> 
> Rephrase suggestion:
> Unfortunately, it is unclear when the PMC HW bug fixed.
> 
>> of hardware the original PMC HW bug was fixed. Although, it was fixed
>> soon after the first introduction of the PMC. Base on this, we assume
> 
> Based
> 
>> that the buggy platforms are less likely to be in used, and it should
> 
> in use
> 
>> be relatively safe to remove this legacy logic.
> 
>>
>> Link: 
>> https://lore.kernel.org/linux-iommu/alpine.LNX.3.20.13.2006030935570.3181@monopod.intra.ispras.ru/ 
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
>> Cc: Tj (Elloe Linux) <ml.linux@elloe.vision>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>> Cc: Alexander Monakov <amonakov@ispras.ru>
>> Cc: David Coe <david.coe@live.co.uk>
>> Cc: Paul Menzel <pmenzel@molgen.mpg.de>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   drivers/iommu/amd/init.c | 24 +-----------------------
>>   1 file changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 648cdfd03074..247cdda5d683 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct 
>> acpi_table_header *table)
>>       return 0;
>>   }
>> -static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 
>> cntr,
>> -                u8 fxn, u64 *value, bool is_write);
>> -
>>   static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>>   {
>> +    u64 val;
>>       struct pci_dev *pdev = iommu->dev;
>> -    u64 val = 0xabcd, val2 = 0, save_reg = 0;

Why not leave this u64 val here? Having the pdev assignment as the
first line makes it easier to read/follow.

>>       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))

Aha - this is going away anyway. Please ignore my comment on 1/2
about parenthesis around (val != val2) being unnecessary.

>> -        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);
>> @@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct 
>> amd_iommu *iommu)
>>       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,
>>
> 

thanks,
-- Shuah