Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32262 )
Change subject: vboot: refactor OPROM code ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32262/6/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32262/6/src/security/vboot/misc.h@40 PS6, Line 40: uint32_t buffer_size; Well, this doesn't achieve anything since the compiler just adds 16 byte padding (and in fact we should always write serializable data structures in a way that won't generate implicit padding, just to be sure). I meant reducing buffer_size as well (or not do it at all).
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.)