Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32262 )
Change subject: vboot: refactor OPROM code ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/32262/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32262/4/src/security/vboot/misc.h@40 PS4, Line 40: uint32_t buffer_size;
While we're on the topic -- do you have any thoughts on `struct region` in commonlib/region.h? Is there a good reason for it using size_t, or would there be any harm in modifying that code to use u16 or u32? Then we can get rid of selected_region altogether and just use the vanilla struct.
That's maybe a better question for Aaron than for me. You could try uploading a patch to change it and see what he thinks. It definitely needs to be 32 there because it is used for regions in memory or on SPI flash which can both be larger than 64K. Whether 32 vs. 64 matters I'm not sure... we always have a sort of implicit rule to never put anything in memory above 4G in coreboot since there is probably some old code assuming pointers to be 32 bit buried somewhere, but whether we should be adding new code cementing that to a core API is a different question.
My guess is that if you propose this, people will just tell you to use u64 to be safe. And then my heart will weep for all those poor 2-3 extra cycles we waste per region operation on 32-bit CPUs, although in practice it won't make a difference.
(And yes, we could get rid of buffer_offset completely too.)
struct region was never intended to be serialized and I weep for fixed size fields in things that aren't serialized. :)
More broadly struct region sort of conflates two things: pointer addressability and boot media. Since our boot media has traditionally been small the same underlying type works (even on 32-bit machines): 4GiB max addressibility. If you wanted to cover everything then you'd need to implement the equivalent of loff_t to cover large spans.
I'm not sure I see much of a gain of trying to serialize struct region. Once one tries to reuse types like that people don't know all the uses and then if someone makes a change then it suddenly breaks an implicit assumption of size and type -- not very stable. So my preference would be to keep the types the same.