From mboxrd@z Thu Jan 1 00:00:00 1970 From: kgunda@codeaurora.org Subject: Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes Date: Wed, 14 Jun 2017 20:39:19 +0530 Message-ID: <386ea95ea5a03d34981350094ff6044e@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-2-git-send-email-kgunda@codeaurora.org> <20170531003310.GS20170@codeaurora.org> <20170613020943.GR20170@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170613020943.GR20170@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd Cc: Abhijeet Dharmapurikar , Christophe JAILLET , Subbaraman Narayanamurthy , David Collins , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com, linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org On 2017-06-13 07:39, Stephen Boyd wrote: > On 06/12, kgunda@codeaurora.org wrote: >> On 2017-05-31 06:03, Stephen Boyd wrote: >> >On 05/30, Kiran Gunda wrote: >> >>From: Abhijeet Dharmapurikar >> >> >> >>The system crashes due to bad access when reading from an non >> >>configured >> >>peripheral and when writing to peripheral which is not owned by >> >>current >> >>ee. This patch verifies ownership to avoid crashing on >> >>write. >> > >> >What systems? As far as I know we don't have any bad accesses >> >happening right now. If they are happening, we should fix the >> >code that's accessing hardware that isn't owned by them. >> > >> This change greatly improves the debugging effort for developers by >> printing >> a very simple and clear error message when an invalid SPMI access >> occurs >> (due to bad DT configuration, bad bootloader SPMI permission >> configurations, >> or other issues). Without this change, such accesses will cause XPU >> violations >> that crash the system and require extensive effort to decode. > > Right, but they're easily detectable because we would know almost > immediately that something isn't working when we integrate a > change. If you update the DT and it stops working, the DT is bad. > If you update the bootloader and it stops working, the bootloader > is bad, etc. > Ok. Will send a patch to remove this code in the next series. >> >> >>For reads, since the forward mapping table, data_channel->ppid, is >> >>towards the end of the block, we use the core size to figure the >> >>max number of ppids supported. The table starts at an offset of 0x800 >> >>within the block, so size - 0x800 will give us the area used by the >> >>table. Since each table is 4 bytes long (core_size - 0x800) / 4 will >> >>gives us the number of data_channel supported. >> >>This new protection is functional on hw v2. >> > >> >Which brings us to the next question which is why do we need this >> >patch at all? We aren't probing hardware to see what we have >> >access to and then populating device structures based on that. >> >Instead, we're just populating DT nodes that we've hardcoded in >> >the dts files, so I'm a little lost on why we would have a node >> >in there that we couldn't access. Please add such details to the >> >commit text. >> > >> invalid SPMI access occurs due to bad DT configuration, bad >> bootloader SPMI >> permission configurations, or other issues. This change reduces the >> debugging >> effort for developers by printing clear error message when an >> invalid SPMI >> access occurs. > > Well we also take an overhead on every read/write. Sure things > are slow so the overhead is negligible, but the permissions are > on a peripheral id basis, so really we should look into _not_ > populating devices that aren't accessible in the first place. > Then we move the checks out of the read/write path and to a more > logical place whereby we prevent a driver from attempting to even > attach to read or write a register that is protected. Ok. Will remove this code in the next patch series and try to implement it as per your suggestion.