Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 15:
(4 comments)
Thank you Hung-Te for your patience!
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h@26 PS15, Line 26: extern struct vpd_blob g_vpd_blob; : : /* : * This function loads g_vpd_blob CAR_GLOBAL variable. : * The variable is initialized if it was not. : */ : const struct vpd_blob *vpd_load_blob(void); : : /* : * This function gets the base address and size of : * buffers for RO_VPD/RW_VPD binary blobs, and sets : * the struct. : */ : void vpd_get_buffers(struct vpd_blob *blob);
Thanks for all the effort here. I think we're pretty close! […]
Hi Hung-Te, thanks for your review!
The reason I did not do "const struct vpd_blob *vpd_get_blobs(void);" is because static variable cannot be used in romstage. If I use static variable in vpd_premem.c, I get this: Forbidden global variables in romstage: ffffff00 b vpd_blob
Therefore CAR has to be relied on (for romstage). The current overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage before FSP-M execution, or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blobs are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a struct vpd_blob variable. ** The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. ** The variable gets contents obtained from CBMEM.
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@106 PS15, Line 106: -
remove the extra space
Done
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@201 PS15, Line 201: size
add a "assert(size > 0)" before calling the MIN.
Done
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... File src/drivers/vpd/vpd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... PS15, Line 83: blob
should we set blob->initialized to True ?
There is no need because initialized field is not necessary for the blob variable in stack. The blob variable is created when vpd_find() is called, and is discard after vpd_find() execution finishes; In other words, it has limited scope, it is fine not to set initialized field. This field needs to be set for CAR_GLOBAL variable, since in romstage, there is no similar way as RAMSTAGE_CBMEM_INIT_HOOK(), and initialized field is needed to tell whether the CAR_GLOBAL was ever initialized.