Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
Was this supposed to be just: […]
I wasn't looking that closely. Sorry. I was thinking this is the same sequence as the PxRcConfig below. Yes, that seems quite problematic. And it appears we're just using the NULL pointer as the config (if FSP is honoring it sitting at address 0). Or it's all ignored.
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG);
Yes, it's sort of fine, except made assumption ARRAY_SIZE(PxRcConfig)<=PCH_MAX_IRQ_CONFIG.
Correct. Static analysis tools will totally catch it as well as the compiler. The assumption is actually flipped, though:
PCH_MAX_IRQ_CONFIG <= ARRAY_SIZE(PxRcConfig)
Ideally we wouldn't duplicate things, but then we'd have to carry the dependency from fsp header file all the way everywhere within coreboot code.