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