On Tue, Aug 24, 2021 at 12:30 PM Alyssa Rosenzweig
<alyssa.rosenzweig@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
