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?

View Change

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?

• File src/lib/imd.c:

  • 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:

  • Patch Set #16:

    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:

  • Patch Set #16:

    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.