lkml.org 
[lkml]   [2017]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes
    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 <adharmap@codeaurora.org>
    > >>
    > >>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

    \
     
     \ /
      Last update: 2017-06-13 04:10    [W:6.467 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site