Patch Set 1:

(11 comments)

Can you try to find a STARTS_IN_ROMSTAGE board to test this on to make sure you're not breaking that path? I'm not sure if any of them still works... I used to keep testing stuff on Falco for a while, so if you can find one of those (or other Slippy variant like Peppy, Leon, Wolf) it might work. Otherwise, it might also work to just hack up the Kconfig of a newer board from STARTS_IN_BOOTBLOCK to STARTS_IN_ROMSTAGE.

Yes, I will find a STARTS_IN_ROMSTAGE board as you suggested.

However, since we don't necessarily care about old boards running on ToT, and since there would conceivably be no reason for new boards to start vboot in romstage, I wonder whether we can just deprecate STARTS_IN_ROMSTAGE and continue simplifying some code?

View Change

11 comments:

• File src/security/vboot/common.c:

  • Patch Set #1, Line 68:

    If you eliminate this CBMEM ID please remove it from <commonlib/cbmem_id. […]

    Seems logical to just remove it.

  • Patch Set #1, Line 151:

    There's another call to this function in vboot_loader.c that you also need to remove.

    Right, thanks for the reminder!

  • Patch Set #1, Line 43: static struct vb2_working_data *vb2_wd = NULL;

    If this is just supposed to be a cache for vboot_get_working_data() and not supposed to be accessed […]

    Well, it's set down in vb2_store_working_data so that it points to the CBMEM copy for any other code that might run afterwards in romstage.
    If you have any other ideas of how to structure this I'm open to suggestions.

  • Patch Set #1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE

    Please never use preprocessor #if where a C if () would also work. […]

    The reason I used preprocessor conditionals --
     * ramstage complained of undefined reference to _vboot2_work
     * verstage complained of undefined reference to cbmem_find

    For the version that you wrote, cbmem_find doesn't seem to be a problem (maybe it gets optimized out for verstage?), but I'm encountering the same problem with undefined _vboot2_work.

    I wonder if we should just make _vboot2_work available to ramstage onward?

  • Patch Set #1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);

    Can we add another option VBOOT_MIGRATE_WORKING_DATA (which would be default y if ARCH_X86, otherwis […]

    (1) How long can we expect the data to stay in SRAM? Will it be cleared after firmware finishes running?
    (2) This would make code a little bit more fragmented. The cleanest way I can think of is to create two CBMEM entries -- one called CBMEM_ID_VBOOT_WORKBUF_PTR (always exists), and one called CBMEM_ID_VBOOT_WORKBUF (sometimes exists). PTR points to either CBMEM or SRAM. Is it worth it?

  • Patch Set #1, Line 62: /* _vboot2_work_size is accessible until end of romstage */

    How does this work for the STARTS_IN_ROMSTAGE case? I don't think the _vboot2_work_size label is ava […]

    Still need to think about this...

  • Patch Set #1, Line 66: static struct selected_region *vb2_selected_region(void)

    nit: arguable if this still needs to be a separate function if the implementation is so trivial.

    Perhaps we can leave it for now? IMHO it's arguable whether or not it should be stored as part of the vb2_working_data struct at all.

  • Patch Set #1, Line 135: vb2_store_working_data

    nit: maybe vb2_migrate_working_data() would be more accurate?

    IMHO migrate implies some kind of transformation of the data. What about copy? Move?
    Any thoughts?

  • Patch Set #1, Line 146: vb2_working_data_size

    This should just be (buffer_offset + workbuf_used), no need to copy the leftover garbage near the en […]

    Re. your first point -- Seems like on creating a vb2_context, the memory following it is always set to 0. If we only copy buffer_offset + workbuf_used, then we'd potentially have garbage instead of 0's afterwards (in CBMEM version). Now, this may not be an issue if:

    • Code that writes to the workbuf doesn't assume it was already 0'd
    • We don't need to write any more data to the workbuf after this point

    But I'm not sure if these two statements are true.

    Re. your second point -- Still trying to decide the best thing to do here. Seems rather awkward to have vb2_shared_data followed by the rest of the workbuf, of which nobody is quite sure of the contents, and whether or not all of it is needed for the vb2_shared_data to be restored (in the next binary). But maybe as part of the "import vb2_shared_data object from previous binary" function, we can just take the relevant parts (pointed to in vb2_shared_data with offset+size) and defragment them together.

  • Patch Set #1, Line 155: SRAM

    SRAM/CAR (also, CBMEM usually capitalized in prose)

    Thanks for the tip. Probably not the right place to ask, but is SRAM and CAR (presumably cache-as-RAM) one and the same thing? Or is CAR only *sometimes* SRAM?

  • Patch Set #1, Line 157: static void vb2_store_cbmem(int unused)

    nit: I'm not really sure why this ever needed to be a separate function... […]

    Previously -- yes I'm not entirely sure either. For the future though -- I was going to add something called vb2_store_shared_data_ptr() (you can probably guess its purpose) after the vb2_store_working_data call. Of course they could be merged into the same function, but it seems somewhat elegant to separate them.

To view, visit change 31329. To unsubscribe, or for help writing mail filters, visit settings.