Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40520 )
Change subject: drivers/smmstore: Implement SMMSTORE version 2 ......................................................................
Patch Set 20:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40520/16/src/commonlib/include/comm... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/40520/16/src/commonlib/include/comm... PS16, Line 504: byte
bytes
Done
https://review.coreboot.org/c/coreboot/+/40520/16/src/commonlib/include/comm... PS16, Line 500: uint32_t num_blocks; /* Number of writeable blocks in SMM */ : uint32_t block_size; /* Size of a block in byte. Default: 64 KiB */ : uint32_t mmap_addr; /* MMIO address of the store for read only access */ : uint32_t com_buffer; /* Physical address of the communication buffer */ : uint32_t com_buffer_size; /* Size of the communication buffer in byte */ : uint8_t apm_cmd; /* The command byte to write to the APM I/O port */ : uint8_t unused[3]; /* Set to zero */
nit: line up the comments
Done
https://review.coreboot.org/c/coreboot/+/40520/14/src/drivers/smmstore/ramst... File src/drivers/smmstore/ramstage.c:
https://review.coreboot.org/c/coreboot/+/40520/14/src/drivers/smmstore/ramst... PS14, Line 63: 0xb2
could use APM_CNT here but you'd have to pass another register like edx.
Done
https://review.coreboot.org/c/coreboot/+/40520/16/src/drivers/smmstore/ramst... File src/drivers/smmstore/ramstage.c:
https://review.coreboot.org/c/coreboot/+/40520/16/src/drivers/smmstore/ramst... PS16, Line 62: $0xb2
APM_CNT ?
Done
https://review.coreboot.org/c/coreboot/+/40520/16/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/40520/16/src/drivers/smmstore/store... PS16, Line 469: /* Better safe than sorry */ : if (!store_initialized) { : printk(BIOS_ERR, "smm store: No com buffer installed yet\n"); : return -1; : } : : if (lookup_store(&store) < 0) { : printk(BIOS_ERR, "smm store: lookup failed\n"); : return -1; : } : : if ((block_id * SMM_BLOCK_SIZE) >= region_device_sz(&store)) { : printk(BIOS_ERR, "smm store: block ID out of range\n"); : return -1; : }
this is repeated 3 times, consider refactoring
Done
https://review.coreboot.org/c/coreboot/+/40520/14/src/include/smmstore.h File src/include/smmstore.h:
https://review.coreboot.org/c/coreboot/+/40520/14/src/include/smmstore.h@39 PS14, Line 39: SMMSTORE of at least 256KiB of size
maybe add a compile time check of CONFIG_SMMSTORE_SIZE if CONFIG_SMMSTOREV2 is enabled.
This requirement only applies when using SMMSTOREv2 with EDK2 FaultTolerantWrite. See comment on PS1.
Turned most of the comment into documentation, and clarified this part.
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.
There are usually 3 regions used by EDK. […]
Thank you for the explanation. I've turned the ever-growing comment into documentation.