Fix Process Loading with App State Section #819

adkinsjd · 2018-03-18T08:25:35Z

Pull Request Overview

Fix process loading with app state section

This commit allows processes using App State to properly load by:

Fixing elf2tbf so that the size of the app_state region is accounted for and noted in a tbf header as a protected region.
Changing proccess.rs so that it takes this protected region into account when calculating the init address of the process.
Moving the AppState region in the linker file so that it doesn't improperly offset the other sections.

Testing Strategy

Testing still required.

TODO or Help Wanted

Processes using App State now load without crashing. App State does not work because the app state section is still marked as a 'protected region' by process.rs. We should discuss the intent behind the protected region and either move the protected region to include the app state section or make a new region for app state.

Documentation Updated

Kernel: Updated the relevant files in /docs, or no updates are required.
Userland: Added/updated the application README, if needed.

Formatting

Ran make formatall.

This commit allows processes using App State to properly load by: - Fixing elf2tbf so that the size of the app_state region is accounted for and noted in a tbf header as a protected region. - Changing proccess.rs so that it takes this protected region into account when calculating the init address of the process. - Moving the AppState region in the linker file so that it doesn't improperly offset the other sections.

bradjc · 2018-03-18T23:25:40Z

So if I'm understanding this correctly, these apps didn't work because the init_fn_offset was essentially pointing to the wrong place?

The purpose of protected_size is to allow the kernel to define space at the beginning of an app's flash region that the app can't modify (and the kernel could then use for whatever persistent state it might want). So I think this is exactly the mechanism you do not want to use for the AppState region.

I thought that the App State region would come through as a writeable_flash_region that the app can get the bounds through the memop flash call.

brghena · 2018-03-19T04:55:18Z

There were two problems when using a writeable flash region via the App State mechanism:

The size of the writeable region (which is after the TBFHeader, but before the Text Segment starts) was not being taken into account when setting up the entry point for the app or informing it where the beginning of its Flash region is. This meant that after running a lot of no-ops, the code would think it's internal memory management header was in that writeable region and load its memory segments incorrectly before inevitably crashing. This needs a fix either in ELF2TBF, process.rs, or both.
The .app_state region was marked in the LD file as after the Text Segment but before the GOT Segment in Flash. This meant that even if an app could access its memory management header correctly, it would have the wrong address for the GOT. This needs a fix in the userland_generic.ld file.

I agree that we misunderstood the purpose of the protected region when we gave a first crack at this last night. I'm going to update this with another shot at it.

Removed need for ELF2TBF to handle it

This is actually where ELF2TBF puts it

brghena · 2018-03-19T06:12:20Z

Alright, I just revised this and I believe it's doing things "the right way" now. Still untested though.

bradjc

I think, but am not entirely sure, that the kernel doesn't need to change and instead elf2tbf needs to change.

bradjc · 2018-03-19T14:14:00Z

kernel/src/process.rs

@@ -375,7 +375,7 @@ impl TbfHeader {
        match *self {
            TbfHeader::TbfHeaderV1(hd) => hd.entry_offset,
            TbfHeader::TbfHeaderV2(hd) =>
-                hd.main.map_or(0, |m| m.init_fn_offset) + (hd.base.header_size as u32),
+                hd.main.map_or(0, |m| m.init_fn_offset) + (hd.base.header_size as u32) + self.get_total_writeable_flash_size(),


I think per the documentation of the TBF header, we don't want to change this. There is no requirement that apps put their writeable flash before the text or in one contiguous block. init_fn_offset should just be set correctly when the header is generated (e.g. by elf2tbf).

bradjc · 2018-03-19T14:14:37Z

kernel/src/process.rs

@@ -428,6 +428,15 @@ impl TbfHeader {
            _ => (0, 0),
        }
    }
+
+    /// Get the total size of writable flash regions.
+    fn get_total_writeable_flash_size(&self) -> u32 {


This function is fine, but it will probably give people the impression that the writeable flash regions are contiguous, which need not be the case.

This is definitely based on an assumption that all writeable flash regions are contiguous and between the TBFHeader and application header. If I'm not allowed to make those assumptions though, I'm not sure how to fix this problem.

crt0.c isn't given any information about the existence (or not) of writeable flash regions. And the current implementation of writeable flash regions places them in Flash immediately after the TBFHeader but before the application header. But, crt0.c assumes that the very first thing in the text segment is its own application header, screwing everything up.

Maybe this could be fixed by placing the .app_state section after the application header but before .start?

Why does the app state affect crt0 at all? The app/crt0 header should be relative to _start (or its own text section start) which it should know. Having app state after the TBFheader and before _text shouldn't be any different than just having a much larger TBFheader.

And apps can get info on writeable flash regions through the memop syscall.

bradjc · 2018-03-19T14:15:15Z

kernel/src/process.rs

+\r\n             │ App State    {:6}                        A\
+  \r\n  {:#010X} ┼─────────────────────────────────────────── S\
+\r\n             │ Protected    {:6}                        H\
+  \r\n  {:#010X} ┴───────────────────────────────────────────\


This might be fine, but again makes assumptions about the app and how it laid out its binary.

We could pull this out if there's no guarantee of app_state placement. I would just have to ensure that the app_flash region and address range includes the writeable region, if it exists.

bradjc · 2018-03-19T14:16:22Z

userland/userland_generic.ld

+    {
+        KEEP (*(.app_state))
+    } > FLASH =0xFF
+


This change shouldn't matter in practice. It's up to elf2tbf to decide what order things are put in.

Unfortunately, that's not correct. If you look at https://github.com/helena-project/tock/blob/b0579e6d710e9b390220a2d7577215684a050a3d/userland/userland_generic.ld#L52 you'll see that the application header is specified as offsets from the beginning of text in the LD file. With .app_state where it was, the GOT offset was off by 1024 Bytes.

Ah good point. I know we discussed this before but it's weird for the linker to be doing this work since elf2tbf has to follow its lead, rather than just requiring elf2tbf to set those values. Seems like this kind of thing might keep happening.

bradjc · 2018-04-06T13:16:24Z

Would changing elf2tbf to put the app state after the text segment (as the linker file expects) resolve this issue? That would be a pretty easy fix and wouldn't involve changing the kernel at all. Is there any reason the app state has to be between TBF header and app text?

brghena · 2018-04-06T17:29:24Z

Sorry about the delay here, I've been busy with a paper deadline.

I believe that moving the app state to after other things in flash would fix the problem. You would actually have to move it to the end of application flash, after the initializations for the data segment too.

bradjc · 2018-04-08T16:44:42Z

When you get a chance you should try #871 which should fix this issue. It should properly insert the appstate section in the binary (before the text section so that when you recompile it doesn't move) while ensuring the pic fixup still works. It does this all in how the applications are generated and doesn't need any kernel changes.

bradjc · 2018-04-26T15:39:34Z

This should be fixed by #871, and if it isn't we need to improve #871.

adkinsjd added 2 commits March 18, 2018 00:47

uncomment the app state section

b2f23f4

adkinsjd added the Work in Progress PR that is still being updated, feedback is likely helpful. label Mar 18, 2018

brghena added 3 commits March 18, 2018 23:06

Account for writeable flash size in process

b201ec3

Removed need for ELF2TBF to handle it

Move app state to beginning of flash (before text)

8619659

This is actually where ELF2TBF puts it

Un-touch the last line of userland

b0579e6

bradjc requested changes Mar 19, 2018

View reviewed changes

ppannuto mentioned this pull request Apr 5, 2018

Kernel Panic on Apps (or some segment of them) too large #864

Closed

bradjc closed this Apr 26, 2018

alevy deleted the fix_appstate_offset branch April 26, 2018 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Process Loading with App State Section #819

Fix Process Loading with App State Section #819

adkinsjd commented Mar 18, 2018

bradjc commented Mar 18, 2018

brghena commented Mar 19, 2018 •

edited

brghena commented Mar 19, 2018

bradjc left a comment

bradjc Mar 19, 2018

bradjc Mar 19, 2018

brghena Mar 19, 2018

bradjc Mar 19, 2018

bradjc Mar 19, 2018

brghena Mar 19, 2018

bradjc Mar 19, 2018

brghena Mar 19, 2018

bradjc Mar 19, 2018

bradjc commented Apr 6, 2018

brghena commented Apr 6, 2018

bradjc commented Apr 8, 2018

bradjc commented Apr 26, 2018

Fix Process Loading with App State Section #819

Fix Process Loading with App State Section #819

Conversation

adkinsjd commented Mar 18, 2018

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

bradjc commented Mar 18, 2018

brghena commented Mar 19, 2018 • edited

brghena commented Mar 19, 2018

bradjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradjc commented Apr 6, 2018

brghena commented Apr 6, 2018

bradjc commented Apr 8, 2018

bradjc commented Apr 26, 2018

brghena commented Mar 19, 2018 •

edited