Hi Adrian,
Great work on the devcoredump support! This is really cool to see coming
along, thank you! I've left a few notes below:
> + if (panfrost_dump_registers[i] >= JS_HEAD_LO(0) &&
> + panfrost_dump_registers[i] <= JS_CONFIG_NEXT(0))
> + js_as_offset = slot * 0x80;
> + else if (panfrost_dump_registers[i] >= AS_TRANSTAB_LO(0) &&
> + panfrost_dump_registers[i] <= AS_STATUS(0))
> + js_as_offset = ((as_nr) << 6);
I'm not a fan of the magic numbers. Do you think it makes sense to add
#define JS_SLOT_STRIDE 0x80
#define MMU_AS_SHIFT 0x6
in the appropriate places in panfrost_regs.h, reexpress the existing
#defines in terms of those
#define JS_HEAD_LO(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x00)
...
#define JS_FLUSH_ID_NEXT(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x70)
...
#define MM_AS(as) (0x2400 + ((as) << MMU_AS_SHIFT)
and then use those here?
Also, drop the parans around (as_nr), this isn't a macro.
> + /* Add in the active buffer objects */
> + for (i = 0; i < job->bo_count; i++) {
> + dbo = job->bos[i];
> + file_size += dbo->size;
> + n_bomap_pages += dbo->size >> PAGE_SHIFT;
> + n_obj++;
> + }
Strictly, I don't think this is right -- what happens if the CPU is
configured to use 16K or 64K pages? -- however, that mistake is pretty
well entrenched in panfrost.ko right now and it doesn't seem to bother
anyone (non-4K pages on arm64 are pretty rare outside of fruit
computers).
That said, out-of-context there looks like an alignment question. Could
we add an assert for that, documenting the invariant:
WARN_ON(!IS_ALIGNED(dbo->size, PAGE_SIZE));
> + iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
> + __GFP_NORETRY);
> + if (!iter.start) {
> + dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");
> + return;
> + }
> ...
> + memset(iter.hdr, 0, iter.data - iter.start);
Why are we using __GFP_NOWARN and __GFP_NORETRY? Why not plain vmalloc?
Also, why vmalloc instead of vzalloc? (Or adding __GFP_ZERO to the list
of __vmalloc flags if __GFP_NOWARN/__GFP_NORETRY are really needed?) Are
there relevant performance or security considerations?
> +/* Definitions for coredump decoding in user space */
> +#define PANFROSTDUMP_VERSION_1 1
I'm not a fan of an enum that just represents a number. Using the
numbers directly means we can compare them in a natural way. Also, using
a major/minor split like Steven suggested can help with semantic
versioning.
Cheers,
Alyssa
