Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] iommu: amd: Fix IOMMU perf counter clobbering during init
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Joerg Roedel <joro@8bytes.org>
Cc:     iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20200114151220.29578-1-skhan@linuxfoundation.org>
 <20200117100829.GE15760@8bytes.org>
 <42c0a806-9947-1401-9754-8aa88bd7062f@amd.com>
 <24a46b0f-33d7-5f94-661a-80f035213892@linuxfoundation.org>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <d7b8aa31-ded5-082e-0324-91b6277507ba@linuxfoundation.org>
Date:   Thu, 23 Jan 2020 14:27:15 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <24a46b0f-33d7-5f94-661a-80f035213892@linuxfoundation.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 1/21/20 8:32 AM, Shuah Khan wrote:
> On 1/20/20 7:10 PM, Suravee Suthikulpanit wrote:
>> On 1/17/2020 5:08 PM, Joerg Roedel wrote:
>>> Adding Suravee, who wrote the IOMMU Perf Counter code.
>>>
>>> On Tue, Jan 14, 2020 at 08:12:20AM -0700, 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>
>>>> ---
>>>>   drivers/iommu/amd_iommu_init.c | 22 ++++++++++++++++------
>>>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>> Suravee, can you please review this patch?
>>>
>>
>> This looks ok. Does this fix certain issues? Or is this just for sanity.
> 
> I didn't notice any problems. Counters aren't writable on my system.
> However, it certainly looks like a bog since registers aren't restored
> like in other places in this file where such checks are done on other
> registers.
> 
> 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?
> 
> 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.
> 
>>
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Joerg,

Please don't pull this in. I introduced a bug in this patch. It always
returns amd_iommu_pc_present false even when it can write to the
counters. My bad.

I will send v2.

thanks,
-- Shuah