[coreboot-gerrit] [S] Change in coreboot[main]: CFR: Add min/max/step values and hex display flag for number options

[Filip Brozovic (Code Review)]

Attention is currently required from: Angel Pons, Matt DeVillier, Sean Rhodes.

Filip Brozovic has posted comments on this change by Filip Brozovic. ( https://review.coreboot.org/c/coreboot/+/86039?usp=email )

Change subject: CFR: Add min/max/step values and hex display flag for number options
......................................................................

Patch Set 2:

(5 comments)

File src/commonlib/include/commonlib/cfr.h:

https://review.coreboot.org/c/coreboot/+/86039/comment/b5688568_1920de4f?usp... :
PS2, Line 94:  * CFR_OPTFLAG_NUMBER_HEX:
            :  *   Displays a numeric option in hexadecimal instead of decimal notation.
            :  *   This flag is only valid for numeric options.
> Flags are meant to be applicable to all option types. This one is not. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/86039/comment/365ad7c7_893573d8?usp... :
PS2, Line 139: uint32_t min;
             : 	uint32_t max;
             : 	uint32_t step;
> This changes the structure layout in a non-backwards compatible way. […]

Would it make more sense to instead put these values (as well as the display flags) into tagged varbinary structs? As far as I can see, any way we try to add a version number, we need to change _some_ struct, making it non-backwards compatible.

If we do add a version number, it probably makes the most sense to add it in the `lb_cfr_header` struct?

Since `min` and `max` are both `uint32_t`, it makes the most sense to make them inclusive, otherwise we have no way to set `0` and `UINT32_MAX`

File src/drivers/option/cfr.c:

https://review.coreboot.org/c/coreboot/+/86039/comment/08851c8d_519f6da5?usp... :
PS2, Line 121: 	option->min = min;
> We should probably check that `min <= max` (both inclusive).

Acknowledged

https://review.coreboot.org/c/coreboot/+/86039/comment/dab0c1b8_f1fbbf17?usp... :
PS2, Line 122: 0xffffffff
> UINT32_MAX

Acknowledged

https://review.coreboot.org/c/coreboot/+/86039/comment/5af3bee7_0393dd9d?usp... :
PS2, Line 123: step
> Let's assume a step size of 0 is equivalent to 1

At least in HII, a step size of 0 means the variable can't be changed with -/+ keys, which makes the behavior different from defining it as 1. Is this behavior something we want to keep?