Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/24942 )
Change subject: WIP / lib: Add blob_provider interface ......................................................................
Patch Set 9:
(15 comments)
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h File src/include/blob_provider.h:
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@29 PS3, Line 29:
why can't we just use struct blob_locator? What do we gain from typedef'ing aside from not writing ' […]
Done
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@48 PS3, Line 48: ID_DATA_NVRAM_VPD_RO_REGION = 17,
put DATA_CODE_SPLIT in here?
Done
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@234 PS3, Line 234: *******
What's the purpose of the fixed width return type? And what are the expected return value?
Done
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@238 PS3, Line 238:
same question as above
Done
https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@242 PS3, Line 242:
cli means what?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c File src/lib/blob_provider.c:
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@27 PS3, Line 27:
Why are we passing a full object instead of just a const pointer?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@30 PS3, Line 30:
this is a weird name -- all caps and also not just cbfs.
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@45 PS3, Line 45: 0) {
Why are we specifying policy on location based on id? […]
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@80 PS3, Line 80: }
Is there a reason to explode out all parameters? Why not jus tpass struct blob_locator?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87 PS3, Line 87:
What's this mean?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87 PS3, Line 87:
this is true only for x86
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@88 PS3, Line 88: *size = region_device_sz(&data);
So we do mmap_full then munmap to make the hook read things?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@94 PS3, Line 94: ev
What does rd stand for?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@104 PS3, Line 104: printk(BIOS_ERR, "Can't use auto mmap function on code blob.");
?
Done
https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c@511 PS3, Line 511: blob_read_map(BLOB_DATA_NVRAM_CMOS_LAYOUT, NULL);
this should really be turned into a read.
Done