Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210824173028.7528-1-alyssa.rosenzweig@collabora.com>
In-Reply-To: <20210824173028.7528-1-alyssa.rosenzweig@collabora.com>
From:   Rob Herring <robh@kernel.org>
Date:   Tue, 24 Aug 2021 17:37:47 -0500
Message-ID: <CAL_JsqJdhVMmroQLjaNVGoW3PJLS3qFRNAVRifGzR-jxNu1Stw@mail.gmail.com>
Subject: Re: [PATCH v2 0/4] drm/panfrost: Bug fixes for lock_region
To:     Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        Tomeu Vizoso <tomeu.vizoso@collabora.com>,
        Steven Price <steven.price@arm.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 24, 2021 at 12:30 PM Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
>
> Chris Morgan reported UBSAN errors in panfrost and tracked them down to
> the size computation in lock_region. This calculation is overcomplicated
> (cargo culted from kbase) and can be simplified with kernel helpers and
> some mathematical identities. The first patch in the series rewrites the
> calculation in a form avoiding undefined behaviour; Chris confirms it
> placates UBSAN.
>
> While researching this function, I noticed a pair of other potential
> bugs: Bifrost can lock more than 4GiB at a time, but must lock at least
> 32KiB at a time. The latter patches in the series handle these cases.
>
> In review of v1 of this series, Steven pointed out a fourth potential
> bug: rounding down the iova can truncate the lock region. v2 adds a new
> patch for this case.
>
> The size computation was unit-tested in userspace. Relevant code below,
> just missing some copypaste definitions for fls64/clamp/etc:
>
>         #define MIN_LOCK (1ULL << 12)
>         #define MAX_LOCK (1ULL << 48)
>
>         struct {
>                 uint64_t size;
>                 uint8_t encoded;
>         } tests[] = {
>                 /* Clamping */
>                 { 0, 11 },
>                 { 1, 11 },
>                 { 2, 11 },
>                 { 4095, 11 },
>                 /* Power of two */
>                 { 4096, 11 },
>                 /* Round up */
>                 { 4097, 12 },
>                 { 8192, 12 },
>                 { 16384, 13 },
>                 { 16385, 14 },
>                 /* Maximum */
>                 { ~0ULL, 47 },
>         };
>
>         static uint8_t region_width(uint64_t size)
>         {
>                 size = clamp(size, MIN_LOCK, MAX_LOCK);
>                 return fls64(size - 1) - 1;
>         }
>
>         int main(int argc, char **argv)
>         {
>                 for (unsigned i = 0; i < ARRAY_SIZE(tests); ++i) {
>                         uint64_t test = tests[i].size;
>                         uint8_t expected = tests[i].encoded;
>                         uint8_t actual = region_width(test);
>
>                         assert(expected == actual);
>                 }
>         }
>
> Changes in v2:
>
> * New patch for non-aligned lock addresses
> * Commit message improvements.
> * Add Steven's tags.
>
> Alyssa Rosenzweig (4):
>   drm/panfrost: Simplify lock_region calculation
>   drm/panfrost: Use u64 for size in lock_region
>   drm/panfrost: Clamp lock region to Bifrost minimum
>   drm/panfrost: Handle non-aligned lock addresses
>
>  drivers/gpu/drm/panfrost/panfrost_mmu.c  | 32 ++++++++++--------------
>  drivers/gpu/drm/panfrost/panfrost_regs.h |  2 ++
>  2 files changed, 15 insertions(+), 19 deletions(-)

For the series,

Reviewed-by: Rob Herring <robh@kernel.org>