Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37871 )
Change subject: soc/intel/common/block: Enable PMC IPC driver ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 257: uint32_t
Shouldn't this be const struct pmc_ipc_cmd *cmd?
do not need to pass the struct in since the command is written in plain form. I used a union to pass this so that it wasn't necessary to then convert to uint32_t before writing.
I don't think this even needs to be a reference anymore it can just be uint32_t
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 634: struct
const... […]
it doesn't need to be passed by reference I will fix that
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 636: uintptr_t
does coreboot frown upon pointer arithmetic on void *? […]
Furquan is right here they should be (void *)(pmcbase + x) will update this to be correct
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 640: inlen > PMC_IPC_DATA_SIZE
What is the significance of this check now? The caller can never pass in more data than what pmc_ipc […]
no significance left it in case some malicious user wants to pass in some invalid length as it is still used in cmd. You are right though its probably not needed.
-
Brandon Breitenstein (Code Review)