[L] Change in coreboot[main]: [RFC] drivers/option: Add option list in cbtables

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Angel Pons, Benjamin Doron, Lean Sheng Tan, Maximilian Brune, Nico Huber, Patrick Rudolph, Sean Rhodes.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74121?usp=email )

Change subject: [RFC] drivers/option: Add option list in cbtables
......................................................................

Patch Set 13:

(6 comments)

File src/drivers/option/cfr.c:

https://review.coreboot.org/c/coreboot/+/74121/comment/7820b565_e75733cc :
PS12, Line 36: 	cfr_str->data_length = strlen(string) + 1;
Do we need to limit the string length?

https://review.coreboot.org/c/coreboot/+/74121/comment/c9b47c49_242b9894 :
PS12, Line 37: 	memcpy(cfr_str->data, string, cfr_str->data_length);
This assumes memory from current is initialized also for the padded bytes that we skip filling with use of ALIGN_UP(). The only runtime impact might be that the logged CRC would unnecessarily change from one build/boot to another.

https://review.coreboot.org/c/coreboot/+/74121/comment/a1031a6a_512afa69 :
PS12, Line 60: 		return 0;
What makes the helptext special to have this test?

https://review.coreboot.org/c/coreboot/+/74121/comment/ba12548c_7f73f531 :
PS12, Line 97: 	current += sm_write_ui_helptext(current, ui_helptext);
Hitting assert() with opt_name and ui_name being NULL seems a bit strong for the purpose, specially in the case if at NULL 0x0 there is non-zero data. Also the line number assert() would print will not be so useful.

Could this use BUG() instead for that case, and return without incrementing current?

File src/drivers/option/cfr.c:

https://review.coreboot.org/c/coreboot/+/74121/comment/5f931ab6_b48fae12 :
PS13, Line 230: 	if (sm_obj->ctor) {
If a dependency evaluates as 0 or does not resolve due to OPTFLAG_SUPPRESS, should this be evaluated already in the constructor to potentially skip adding the option in the stream too?

This would require depth-first walk of the form or some post-processing.

https://review.coreboot.org/c/coreboot/+/74121/comment/dccfa148_520dd051 :
PS13, Line 242: 		if (sm_obj->sm_enum.flags & CFR_OPTFLAG_SUPPRESS)
If an object is referenced as a dependency, is it allowed for it's constructor function to set the SUPPRESS flag? The dep_id would not resolve runtime. I think it also leaves it to the interpretation inside the parser on what to dislay, the referencer should be flagged as SUPPRESS too? Or as READONLY and GRAYOUT?