Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751898AbdFIQJt (ORCPT ); Fri, 9 Jun 2017 12:09:49 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:7805 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751784AbdFIQJq (ORCPT ); Fri, 9 Jun 2017 12:09:46 -0400 Subject: Re: [PATCH v8 6/9] drivers: perf: hisi: Add support for Hisilicon Djtag driver To: Mark Rutland References: <1495457312-237127-1-git-send-email-zhangshaokun@hisilicon.com> <20170608163519.GA19643@leverpostej> <8666a0fa-126d-e4a3-ac4b-7962f5d79942@huawei.com> <20170609154425.GH10665@leverpostej> CC: Shaokun Zhang , , , , , , , , , , , , , From: John Garry Message-ID: Date: Fri, 9 Jun 2017 17:09:25 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20170609154425.GH10665@leverpostej> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.181.152] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020201.593AC848.00AA,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 61dc9d4e3e1b48d4df1e198ee2d2068c Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2877 Lines: 94 Hi Mark, > What happens if the lock is already held by an agent in that case? > > Does the FW block until the lock is released? The FW must also honour the contract - it must also block until the lock is available. This may sound bad, but, in reality, probablity of simultaneous perf and hotplug access is very low. > > Can you elaborate on CPU hotplug? Which CPU is performing the > maintenance in this scenario, and when? Can this block other CPUs until > the lock is released? > > What happens if another agent pokes the djtag (without acquiring the > lock) while FW is doing this? Can this result in issues on the secure > side? > I need to check on this. > [...] > >>> Can you explain how the locking scheme works? e.g. is this an >>> advisory software-only policy, or does the hardware prohibit accesses >> >from other agents somehow? >> >> The locking scheme is a software solution to spinlock. It's uses >> djtag module select register as the spinlock flag, to avoid using >> some shared memory. >> >> The tricky part is that there is no test-and-set hardware support, >> so we use this algorithm: >> - precondition: flag initially set unlocked >> >> a. agent reads flag >> - if not unlocked, continues to poll >> - otherwise, writes agent's unique lock value to flag >> b. agent waits defined amount of time *uninterrupted* and then >> checks the flag >> - if it is unchanged, it has the lock -> continue >> - if it is changed, it means other agent is trying to access the >> lock and got it, so it goes back to a. >> c. has lock, so safe to access djtag >> d. to unlock, release by writing "unlock" value to flag > > This does not sound safe to me. There's always the potential for a race, > no matter how long an agent waits. > >>> What happens if the kernel takes the lock, but doesn't release it? >> >> This should not happen. We use spinlock_irqsave() when locking. >> However I have noted that we can BUG if djtag access timeout, so we >> need to release the lock at this point. I don't think the code >> handles this properly now. > > I was worried aobut BUG() and friends, and also preempt kernels. > > It doesn't sound like it's possible to make this robust. > >>> What happens if UEFI takes the lock, but doesn't release it? >> >> Again, we would not expect this to happen; but, if it does, Kernel >> access should timeout. > > ... which they do not, in this patch series, as far as I can tell. > I noticed this also. > This doesn't sound safe at all. :/ Right, we need to consider if this will fly at all. At this point, we would rather concentrate on our new chipset, which is based on same perf HW architecture (so much code reuse), but uses directly mapped registers and *no djtag* - in this, most of the upstream effort from all parties is not wasted. Please advise. Much appreciated, John > > Thanks, > Mark. > > . >