On 06/12, kgunda@xxxxxxxxxxxxxx wrote: > On 2017-05-31 06:03, Stephen Boyd wrote: > >On 05/30, Kiran Gunda wrote: > >>From: Abhijeet Dharmapurikar <adharmap@xxxxxxxxxxxxxx> > >> > >>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. > > >>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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
