Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40520 )
Change subject: drivers/smmstore: Implement SMMSTORE version 2 ......................................................................
Patch Set 16:
(4 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
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
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 ?
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