[RFC] Tracer bullet for bootloader API

[utzig]

NOT TO BE MERGED YET

This is an initial implementation of an API call table for MCUBoot. Sample changes to make Mynewt compatible, can be seen by looking at the changes on the following branch:

https://github.com/apache/incubator-mynewt-core/compare/master...utzig:bootapi?expand=1

[d3zd3z]

Make this a (separate) #define, and pass the constant into the inline assembly.

[utzig]

@d3zd3z Might be a good idea to group the reqs by type, which could be flash_map, flash_read/write, something else in the future... and increment from a different starting value (0x1000, 0x2000, etc). What do you think?

[d3zd3z]

I'm not sure the compiler is required to preserve r4 and r5 after this asm block, especially since these registers are used for the calling convention. I'd suggest either moving the jump into this sequence, or at least on arm, not botherer with it, and just doing

((void (*)(void))start)(MCUBOOT_BOOT_MAGIC, ptrtoblock);

The ARM register convention passes the first two arguments in r4 and r5. This will generate a bl or blx instruction, but having a return address in lr doesn't really hurt anything (we're already doing this).

Not sure how this would work on x86, since by convention the args are passed on the stack. 'fastcall' will use registers for the first two, so this might work:

((void __fastcall (*)(uint32_t, uint32_t))start)(MCUBOOT_BOOT_MAGIC, (uint32_t)ptrblock);

(with __fastcall possibly needing to be defined, on gcc it is __attribute__((__fastcall__)).

[utzig]

Adding the clobbers there means the registers will not be reused for any "asm" stuff apart from the explicit usage: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers. Not explicitly stated but it would not make much sense IMO if they were not preserved.

According to http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf r0-r3 are used for parameters and r4-r7 for local variables inside the called function.

Btw, the asm block could be updated to just jump/branch directly instead of having to call the function in C. What do you think?

[utzig]

Btw, I will test the parameter passing stuff to see if it works.

[d3zd3z]

The thing is the "clobbers" won't be used to prep and post the asm, but the compiler would be free to use those registers after, including for the call following it. It turns out that we're ok here because the call has no args, so the compiler feels no need to put anything else in the registers.

But, I'd be more comfortable if it was explicit, either putting the jump in the asm itself (along with the arguments), or just using a regular call with arguments.

[utzig]

The thing is the "clobbers" won't be used to prep and post the asm, but the compiler would be free to use those registers after, including for the call following it. It turns out that we're ok here because the call has no args, so the compiler feels no need to put anything else in the registers.

Well, as the ABI document I pointed before says: r0-r3 are used for passing first 4 parameters, extra parameters go in the stack, so we're technically OK by using r4-r5

But, I'd be more comfortable if it was explicit, either putting the jump in the asm itself (along with the arguments), or just using a regular call with arguments.

Yeah, I agree the calling in assembly is safer, I'll try that.

For completeness I tried to check the generated asm and confirm if it is done as defined in the previously mentioned ABI

int fn(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7)
{
    int a = x0;
    int b = x1;
    return a == b;
}

int main(void)
{
    fn(0, 1, 2, 3, 4, 5, 6, 7);
    return 0;
}

$ arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb-interwork -mthumb -S test.c

Relevant assembly (function call)

        ...
        movs	r3, #5
	str	r3, [sp, #4]
	movs	r3, #4
	str	r3, [sp]
	movs	r3, #3
	movs	r2, #2
	movs	r1, #1
	movs	r0, #0
	bl	fn
        ...

And it seems that r4-r5 are never touched!

[d3zd3z]

Does this patch make use of this macro?

Given that the do_boot functions are target specific, I'm not sure we need to hide all of this in macros, and just put it in the code in the appropriate function.

[utzig]

The idea is to export a macro to "register" the boot api stuff before jumping into the app. do_boot is zephyr specific, but exporting the macro enables mynewt to "include" it in hal_system_start before jumping to the app.

[d3zd3z]

I find it confusing to leave things in registers after this though. It may work now, but I don't think it is guaranteed to work. I think both the register setup as well as the call are arch specific and should really be together, not split like this.

e.g., other processors may need something other than a call to jump into the app, maybe some explicit instructions and a special kind of jump.

[d3zd3z]

I'm going to put this comment here as well, so it doesn't get lost:

After a discussion with [redacted] today, I think the things we'll need to communicate are:

• Partitions, at least the partitions the bootloader cares about (the slots and scratch).
• Version information.
• Separate version information about the trailer, so know how it should be marked.
• How mcuboot is configured (swap, upgrade only, run slot)
• Which slot was run (needed for OTA with dual slot solution).

Things I'm less sure about:

• Other partitions
• sector map and write alignment (the drive really should know these)
• ???

[d3zd3z]

A lot has changed since we've originally reviewed this. At least with Zephyr, we have a fairly solid method of making sure the partitions match between MCUboot and the app (device tree), although it is done at build time, and can still be messed up.

TF-M's fork of MCUboot has implemented a communication mechanism that they plan to upstream, but it has a fairly different purpose. It is intended to communicate measurements that will be part of an attestation token. We could rework these patches to fit those requirements, or just bring in fresh code.

[d3zd3z]

Closing, as this seems to mostly just be out of date.