8 comments:
File src/security/vboot/common.c:
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.
Patch Set #1, 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?
Patch Set #1, 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.
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?
Patch Set #1, 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).
Patch Set #1, 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.
Patch Set #1, 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".
Patch Set #1, 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.
Patch Set #1, 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...
To view, visit change 31329. To unsubscribe, or for help writing mail filters, visit settings.