Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 107: int vboot_is_slot_a(const struct vb2_context *ctx) nit: should probably be a static inline
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device
This would be a separate CL if we decide to fix it but -- I wonder why we have both security/vboot/c […]
Agreed, merging sounds fine to me. Also we should get rid of vboot_named_region_device(_rw) and just use fmap_locate_area_as_rdev(_rw) directly.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: const struct vb2_context *ctx
I wonder if we really need to pass the context object around in coreboot (same for the function belo […]
I'd say it depends on how it's called. If every caller already has a context anyway, I think doing it this way is fine. (Honestly, I'm not sure why we need this function at all, I don't think checking a single flag needs to be wrapped.)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 76: const struct vb2_context *ctx = vboot_get_context(); This is only legal to call after vboot_logic_executed() returns true, so you have to move it below that check.