Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40520 )
Change subject: [RFC]drivers/smmstore: Implement SMMSTORE version 2 ......................................................................
Patch Set 1:
(5 comments)
Looks quite good to me. Don't forget Documentation/drivers/smmstore.md when it's done.
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/smi.c File src/drivers/smmstore/smi.c:
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/smi.c@... PS1, Line 63: case SMMSTORE_CMD_INFO: { Given that less SMM code is typically better/desirable, would you want to have a Kconfig option for which 'versions' you compile (1, 2 or both for max compatibility)?
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/store.... PS1, Line 270: static struct smmstore_params_info info_params; So smmstore_update_info has to be called before any of the other operations can be used? Would you want an error code on the other operations if that is not the case?
Can't read/write/erase ops do also that when it's not initialized by smmstore_update_info() given that the information is based on something constant (FMAP region)?
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/store.... PS1, Line 272: smmstore_update_info You probably want to check if the base of the region is 64KiB aligned? Maybe the erase ops of region_device already do that, not sure...
https://review.coreboot.org/c/coreboot/+/40520/1/src/drivers/smmstore/store.... PS1, Line 304: if (offset >= info_params.block_size || block_id >= info_params.num_blocks) { : printk(BIOS_WARNING, "access out of region requested\n"); Maybe some more fine grained error messages could be useful when debugging things?
https://review.coreboot.org/c/coreboot/+/40520/1/src/include/smmstore.h File src/include/smmstore.h:
https://review.coreboot.org/c/coreboot/+/40520/1/src/include/smmstore.h@40 PS1, Line 40: * Version 2 of the SMM requires as SMMSTORE of at least 256KiB of size, that : * has to be aligned to 64KiB. : * This allows the payload to store raw data in the SMMSTORE flash region. : * This can be used by a FaultTolerantWrite implementation, that uses at least : * two regions in an A/B update scheme. Why is 256KiB needed if you have A/B (2) regions of 64KiB? Or is it typical to need 2 x 64KiB region for storing things for which you want an A/B scheme?
I presume that 64KiB is because coreboot only implements 64KiB erase ops on some flashes? Might be worth mentioning that?