Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31329 )
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@a68 PS1, Line 68:
Actually, it may be worth keeping (as deprecated) so the cbmem userspace utility can still identify […]
Good point. I'll add a comment to mark it as deprecated.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL;
We could just mark this CAR_GLOBAL, right? Shouldn't hurt?
This may be a silly question, but -- if coreboot already has functionality to migrate data from CAR to CBMEM, why don't we just rely on it for the entire vb2_working_data struct, rather than migrating it ourselves?
Is it because the "lose access to SRAM on some SRAM architectures" case is not covered?
Assuming that this is a bad idea, and we still want to store a global pointer here -- what is the benefit of storing it as a CAR_GLOBAL?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
other than cbmem_possibly_online() in the way we're already using it […]
So far, Julius's original solution + #ifdef __PRE_RAM__ seems to make the most sense. And yes, we still have the option of implementing a helper like cbmem_definitely_online() or creating ENV_PRE_RAM. * The logic between !cbmem_definitely_online implying that we still have access to _vboot2_work seems a bit far. * ENV_PRE_RAM seems like a nice way of getting rid of precompiler logic. There's already a ENV_CACHE_AS_RAM defined in src/include/rules.h, but it's not *quite* the same meaning that we are looking for, since we also want to allow for the SRAM case.
I'll add ENV_PRE_RAM -- does that work for everyone?
I'm also considering switching back to a boolean with CAR_GLOBAL instead of storing a cached pointer.
I have another related question -- why does cbmem_possibly_online logic include a check of CONFIG_VBOOT_STARTS_IN_BOOTBLOCK?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
(1) It depends on the system, but for most we never touch SRAM again after coreboot. […]
(1) Right, unless we want to access the full working_data buffer for some kind of debugging purposes, after booting into the OS. But I can't think of a specific use-case to keep a copy laying around somewhere on these SRAM architectures.
(2) Yes, I'm talking about passing to the payload. Actually it seems somewhat limiting that libpayload doesn't seem to have first-class support for CBMEM. Everything that libpayload needs to access seems to get shoved into the coreboot table. Yes, we could store a pointer directly to the SRAM, which requires writing custom code to write to coreboot table (unfortunately can't use add_cbmem_pointers), and custom code to read from it on the depthcharge side. I suppose that's the most straightforward way to do it at this point.
So we will add a function to coreboot_table.c called lb_vboot_shared_data which will point to vb2_shared_data, regardless its location (SRAM or CBMEM, but probably not CAR).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
I mean... […]
Well... Randall and I were also toying with the idea of giving vb2_context the ability to be cross-binary compatible. In this case, the whole chunk of vb2_context + vb2_shared_data + rest of workbuf would always travel as a contiguous chunk, and could be directly imported downstream by vboot. In this case, it would be convenient if we could directly point to a CBMEM object rather than point to the middle of one.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data
I think we use "migrate" a lot when talking about moving things from pre-RAM to post-RAM (e.g. […]
OK, probably best to follow prior usage of the word then. I will switch to "migrate".
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size
Hmmm... yeah, I'm not sure why we're zeroing the whole thing in vb2_init_working_context() either. […]
Re. the zeroing in vb2_init_working_context: Let's get rid of that step. Zero-filling the vb2_working_data struct is sufficient.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
No, I can't really guess its purpose. […]
Yes, part of the motivation is that coreboot no longer needs to access vb2_shared_data directly. However, another point is to get rid of the whole vboot_handoff step. So we should find a way to send vb2_shared_data downstream directly, rather than transforming it to vboot1 equivalent and wrapping it in a vboot_handoff struct.
We can't just expect depthcharge to know where to find vb2_shared_data within the vb2_working_data structure, which is private to coreboot. I believe it would also be undesirable to teach it how to read this data structure. So we add another entry to CBMEM pointing to the data it needs -- vb2_shared_data. Thus the vb2_store_shared_data_ptr function.
I'll put all this in the design doc and send it out for review...