Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30684 )
Change subject: Use uintptr_t instead of (void *) where it makes sense ......................................................................
Patch Set 16:
(16 comments)
Sorry, some things slipped through my earlier review. Some reasoning for the BERT issue: I think if the address is used as a pointer in a subsystem, we shouldn't drag `uintptr_t` into it. A nit, though, as our ACPI code is full of `unsigned long` for addresses.
We are on the right track, I think: $ # removed casts to pointers: $ git show | grep '^-.**)' | wc -l 69 $ # added casts to pointers $ git show | grep '^+.**)' | wc -l 7
Six of the latter are actually unnecessary, they were three, unrelated casts before. So we could get the added count down to 1, the cast for/in BERT.
I understand that this is hard to split up (would add a lot of intermediate casts). Maybe, to reduce the round-trip time, I could fix my comments by myself and we review both?
https://review.coreboot.org/#/c/30684/16/src/arch/x86/acpi_bert_storage.c File src/arch/x86/acpi_bert_storage.c:
https://review.coreboot.org/#/c/30684/16/src/arch/x86/acpi_bert_storage.c@54 PS16, Line 54: void bert_errors_region(uintptr_t *start, size_t *size) I would prefer not to change this signature, having a `uintptr_t` here seems very odd. Also, it doesn't reduce the number of casts, you have one cast, it seems, either for the return value of cbmem_top() in stoney, or for the memset() argument below.
https://review.coreboot.org/#/c/30684/16/src/arch/x86/acpi_bert_storage.c@79 PS16, Line 79: return (void *)((u8 *)bert_region_base + alloc); OTOH, we could spare us the inner cast here now.
On the third? hand, making `bert_region_base` a `uint8_t *` would also spare us the cast.
https://review.coreboot.org/#/c/30684/16/src/drivers/intel/fsp1_1/stack.c File src/drivers/intel/fsp1_1/stack.c:
https://review.coreboot.org/#/c/30684/16/src/drivers/intel/fsp1_1/stack.c@13... PS16, Line 133: slot = stack_push32(slot, (uint32_t)smm_base | MTRR_TYPE_WRBACK); Do we need the cast?
https://review.coreboot.org/#/c/30684/16/src/drivers/intel/fsp2_0/hob_verify... File src/drivers/intel/fsp2_0/hob_verify.c:
https://review.coreboot.org/#/c/30684/16/src/drivers/intel/fsp2_0/hob_verify... PS16, Line 66: range_entry_end(&tolum), cbmem_top()); `lx` wants an `unsigned long`, cast?
https://review.coreboot.org/#/c/30684/16/src/lib/imd.c File src/lib/imd.c:
https://review.coreboot.org/#/c/30684/16/src/lib/imd.c@401 PS16, Line 401: imdr_init(&imd->sm, 0);
Please mention this additional fixes also in the commit message.
This changes because of the change in imd_init()'s signature. It's not really "additional" but part of the same change.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/cannonlake/cbmem.c File src/soc/intel/cannonlake/cbmem.c:
PS16: Unrelated? Do it in advance?
https://review.coreboot.org/#/c/30684/16/src/soc/intel/cannonlake/smmrelocat... File src/soc/intel/cannonlake/smmrelocate.c:
https://review.coreboot.org/#/c/30684/16/src/soc/intel/cannonlake/smmrelocat... PS16, Line 249: memset((void *)params->ied_base + 1 * MiB, 0, 32 * KiB); Seems unrelated and is wrong, pointer arithmetic on `void *` is undefined (or at least implementation defined). I would leave this function as is. Or you add parentheses around the addition.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/icelake/cbmem.c File src/soc/intel/icelake/cbmem.c:
PS16: Same as cannonlake.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/icelake/smmrelocate.c File src/soc/intel/icelake/smmrelocate.c:
https://review.coreboot.org/#/c/30684/16/src/soc/intel/icelake/smmrelocate.c... PS16, Line 248: memset((void *)params->ied_base + 1 * MiB, 0, 32 * KiB); Same as above.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/quark/romstage/fsp2_0... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/30684/16/src/soc/intel/quark/romstage/fsp2_0... PS16, Line 76: postcar_frame_add_mtrr(&pcf, (uintptr_t) top_of_low_usable_memory, Not directly related, but... yes, it's declared as that ;)
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/memmap.c File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/memmap.c@63 PS16, Line 63: uintptr_t sub_base; You called it `smm_base` above, please align.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/smmrelocate.c File src/soc/intel/skylake/smmrelocate.c:
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/smmrelocate.c... PS16, Line 208: params->smram_base = (uintptr_t)handler_base; Remove cast, please.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/smmrelocate.c... PS16, Line 210: params->ied_base = (uintptr_t)ied_base; Here, too.
https://review.coreboot.org/#/c/30684/16/src/soc/intel/skylake/smmrelocate.c... PS16, Line 257: memset((void *)params->ied_base + 1 * MiB, 0, 32 * KiB); And again.
https://review.coreboot.org/#/c/30684/16/src/soc/sifive/fu540/cbmem.c File src/soc/sifive/fu540/cbmem.c:
https://review.coreboot.org/#/c/30684/16/src/soc/sifive/fu540/cbmem.c@25 PS16, Line 25: FU540_MAXDRAM); Does this fit in a single line now?
https://review.coreboot.org/#/c/30684/16/src/soc/ucb/riscv/cbmem.c File src/soc/ucb/riscv/cbmem.c:
https://review.coreboot.org/#/c/30684/16/src/soc/ucb/riscv/cbmem.c@26 PS16, Line 26: return (base + size); Parents not needed any more.