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 2:
(3 comments)
I agree with Aaron's points, I think it would be cleaner to just remove the selected_region field and everything related to it right away, rather than leaving things in this half-state where some code still looks at that and some doesn't anymore.
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 122: vboot_named_region_device
There's still vbnv_flash() using vboot_named_region_device_rw().
Okay, we should clean that up too then. (I think those functions were written before the more generic fmap_locate_area_as_rdev() existed, but now that it does we should just use that directly.)
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();
Our current assumption is that vboot_get_context() will only ever be called by verstage for the firs […]
Yeah, so more precisely, it's only legal to call it in verstage or later. But for the purposes of this function, vboot_logic_executed() is a sufficient proxy for that.
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... PS2, Line 83: if (!vboot_is_slot_selected()) Just check directly for (ctx->flags & RECOVERY) here instead?