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?
16 comments:
File src/arch/x86/acpi_bert_storage.c:
Patch Set #16, 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.
Patch Set #16, 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.
File src/drivers/intel/fsp1_1/stack.c:
Patch Set #16, Line 133: slot = stack_push32(slot, (uint32_t)smm_base | MTRR_TYPE_WRBACK);
Do we need the cast?
File src/drivers/intel/fsp2_0/hob_verify.c:
Patch Set #16, Line 66: range_entry_end(&tolum), cbmem_top());
`lx` wants an `unsigned long`, cast?
Patch Set #16, 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.
File src/soc/intel/cannonlake/cbmem.c:
Unrelated? Do it in advance?
File src/soc/intel/cannonlake/smmrelocate.c:
Patch Set #16, 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.
File src/soc/intel/icelake/cbmem.c:
Same as cannonlake.
File src/soc/intel/icelake/smmrelocate.c:
Patch Set #16, Line 248: memset((void *)params->ied_base + 1 * MiB, 0, 32 * KiB);
Same as above.
File src/soc/intel/quark/romstage/fsp2_0.c:
Patch Set #16, Line 76: postcar_frame_add_mtrr(&pcf, (uintptr_t) top_of_low_usable_memory,
Not directly related, but... yes, it's declared as that ;)
File src/soc/intel/skylake/memmap.c:
Patch Set #16, Line 63: uintptr_t sub_base;
You called it `smm_base` above, please align.
File src/soc/intel/skylake/smmrelocate.c:
Patch Set #16, Line 208: params->smram_base = (uintptr_t)handler_base;
Remove cast, please.
Patch Set #16, Line 210: params->ied_base = (uintptr_t)ied_base;
Here, too.
Patch Set #16, Line 257: memset((void *)params->ied_base + 1 * MiB, 0, 32 * KiB);
And again.
File src/soc/sifive/fu540/cbmem.c:
Patch Set #16, Line 25: FU540_MAXDRAM);
Does this fit in a single line now?
File src/soc/ucb/riscv/cbmem.c:
Patch Set #16, Line 26: return (base + size);
Parents not needed any more.
To view, visit change 30684. To unsubscribe, or for help writing mail filters, visit settings.