Multiple OS Support #48

chrisguikema · 2022-10-25T23:57:19Z

Test with: seL4/seL4_projects_libs#81
Test with: seL4/camkes-vm-examples#31

components/VM_Arm/src/main.c

components/VM_Arm/src/modules/init_ram.c

components/VM_Arm/configurations/vm.h

components/VM_Arm/src/main.c

components/VM_Arm/configurations/vm.h

axel-h

This looks good to get merged now for me.

chrisguikema · 2023-02-22T18:17:37Z

Thanks for the review, Axel. Appreciate it!

axel-h · 2023-02-22T18:30:55Z

Thanks for the review, Axel. Appreciate it!

The stuff in these PR is really useful and having all these config option do avoid some hacks on our side, too. Also, now that I have looked into this again, this explains some things I have been really wondering about...

axel-h · 2023-02-23T13:50:42Z

components/VM_Arm/configurations/vm.h

-        string linux_ram_paddr_base; \
-        string linux_ram_size; \
-        string linux_ram_offset; \
+        string ram_base; \


Since we are doing a breaking change here anyway, I wonder if we should also change stringto uint64_t here. There seems no point why these are string, except that in the past this was necessary because uint64_t did not exist.

hlyytine · 2023-03-04T10:43:46Z

components/VM_Arm/src/main.c

@@ -856,7 +864,7 @@ static int load_vm(vm_t *vm, const char *kernel_name, const char *dtb_name, cons

    /* Attempt to load initrd if provided */
    guest_image_t initrd_image;
-    if (config_set(CONFIG_VM_INITRD_FILE)) {
+    if (vm_image_config.provide_initrd) {


How about just if (strlen(initrd_name))? Then you could set initrd_name to empty string in CAmkES file to not load any. No need to introduce additional option.

Yes, that would be sufficient to keep the config options lighter. Should we put this into

camkes-vm/components/VM_Arm/configurations/vm.h

Line 82 in 2965240

string initrd_name = "linux-initrd"; \

then:

string initrd_name = ""; /* no inird by default */ \

Sorry @chrisguikema for stealing the show, but let me show how I think this per-VM initrd is handled best: #59.

Figured out we still have 3 special cases for guest's data blobs: kernel, DTB and initrd. I got an idea how to write that in a more generic way and remove lots of code from VM. Please give me a day or two so I can show my suggestion.

I appreciate the review, Hannu, and I also appreciate that you're creating new MRs that address your comments

hlyytine · 2023-03-04T10:52:50Z

components/VM_Arm/src/main.c

@@ -865,7 +873,10 @@ static int load_vm(vm_t *vm, const char *kernel_name, const char *dtb_name, cons
        }
    }

-    if (!config_set(CONFIG_VM_DTB_FILE)) {
+    ZF_LOGW_IF(vm_image_config.provide_dtb && vm_image_config.generate_dtb,


How about getting rid of both these booleans and reserving some magic string for autogeneration. For example, setting dtb_name to <generate> is not a likely filename since it would create havoc in build systems due to < and > being shell redirections. The second case is leaving dtb_name empty and no DTB is loaded. Any other string should be considered a filename.

Actually, there are multiple options, see also my comment #48 (comment), so using your proposal we'd have:

don't provide DTB. Could be dtb_name = ""

provide a fixed TB from dtb_name. Do not modify it. Could be dtb_name = "some.dtb"

generate as DTB from scratch. Could be dtb_name = "<generate>"

generate as DTB based on a give DTB. Should this be handled via dtb_name = "<generate>some.dtb"

How about:

Don't provide DTB: dtb_name = ""

Provide fixed DTB: dtb_name = "some.dtb"

Generate from scratch: dtb_name = "<"

Generate from given DTB: dtb_name = "<some.dtb"

Note dropped > at the end. Now you can just check if first character is <, if yes, use the remaining string (which might be empty) as filename. No need for error prone string copies. Any other character than < would be fine to me, as long as it is unlikely to appear in real world filenames.

That looks nice actually. We just have to clearly document this somewhere then also.

components/VM_Arm/src/main.c

templates/seL4VMParameters.template.c

axel-h · 2023-03-06T21:58:12Z

templates/seL4VMParameters.template.c

+unsigned long initrd_max_size = /*? vm_address_config.get('initrd_max_size') ?*/;
+unsigned long initrd_addr = /*? vm_address_config.get('initrd_addr') ?*/;
+
+/*- if vm_address_config.get('kernel_entry_addr') != '-1' -*/


Since vm_address_config is really new, why do we want to still support "-1" then at all?

It needs to be set or else it will cause an KeyError during the init-build.sh script. So keeping the -1 default value allows for the automatic calculation to function if its not set by the user.

No sure I follow, wan't the point that we want the user to set this explicitly in the configuration, so it's more transparent what happens and there is less magic at the bottom? We could have an explicit linux template that provides defaults for Linux VMs, so users can use this one. Or a flag that says it's a linux kernel, which then enabled these well known defaults and maybe more convenince features. Also, it could be addressed in a meta VM creation system, that has more insight about the actual VMs, like the "vm composer" you have.

I'm saying that every member of the vm_address_config struct must have a value or the init-build.sh script will fail. We want to encourage the users to explicitly set the value, so when it is not set, CAmkES calculates a default, based off linux's offset, and the following warning will appear:

[258/305] Building C object CMakeFiles/vm0.instance.bin.dir/vm0/seL4VMParameters.template.c.obj vm0/seL4VMParameters.template.c:27:2: warning: #warning Entry address has been calculated. Please use vm_address_config.kernel_entry_addr [-Wcpp] 27 | #warning Entry address has been calculated. Please use vm_address_config.kernel_entry_addr | ^~~~~~~

We need the -1 default value because if the user didn't set the value, the configuration wouldn't even be able to compile. The #warning is what encourages the user to set the value.

Ah, I see. So I wonder, should the complete macro VM_INIT_DEF() be deprecated then? It is creating a vm_address_config and a linux_address_config block now, which seems a bit odd. Is it somehow possible to not have it create a vm_address_config block and then print a warning if that it's outdated? And there is another new macro (maybe VM_GENERIC_INIT_DEF()) to be used that creates only a vm_address_config now. And if this new macro is used, the all fields must be set by the user?

I think if we have a version, like Hannu was suggesting, we can only include the necessary attribute. So for now this is okay, but we can make that part of the next discussion.

templates/seL4VMParameters.template.c

templates/seL4VMParameters.template.h

components/VM_Arm/docs/vm_memory_configuration.md

components/VM_Arm/configurations/vm.h

The VMM can run other Operating Systems, so referencing Linux isn't necessarily correct. In order to maintain backwards compatibility, a new template was created to set the address parameters during compile time. The template first pulls from vm_XXXX_config, but if that structure isn't available, it will pull from linux_XXXX_config. Signed-off-by: Chris Guikema <chris.guikema@dornerworks.com>

This commit allows the vm_arm component to run multiple OSs by removing cmake variables and replacing them with camkes variables. Since cmake variables apply to the entire project, this forces a system with two VMs to each apply the CMake variables, which could break one of the VMs. By replacing the variables with camkes flags, the functionality remains, while allowing for more flexibility. Signed-off-by: Chris Guikema <chris.guikema@dornerworks.com>

This markdown document overviews how to configure a guest's address space. Signed-off-by: Chris Guikema <chris.guikema@dornerworks.com>

ram_offset and initrd_max_size were not used in the VMM. Since there is a new configuration block, its safe to remove the attributes Signed-off-by: Chris Guikema <chris.guikema@dornerworks.com>

These are now defined in the camkes configuration and are not needed in the vmlinux.h header files. Signed-off-by: Chris Guikema <chris.guikema@dornerworks.com>

kent-mcleod · 2023-03-07T04:29:28Z

@chrisguikema Can you let me know if you're ready for these 3 PRs to be merged.

chrisguikema · 2023-03-07T04:30:23Z

Yeah I'm all good on my end!

seL4/camkes-vm#48 made this possible. Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

This was referenced Oct 25, 2022

Add Multiple OS Support seL4/camkes-vm-examples#31

Open

libsel4vm: add entries to vm structure seL4/seL4_projects_libs#81

Merged

vm_arm: dereference linux #47

Closed

chrisguikema force-pushed the multiple_os branch from a88fd95 to 6a4faa1 Compare October 26, 2022 00:10

kent-mcleod reviewed Oct 26, 2022

View reviewed changes

components/VM_Arm/src/main.c Outdated Show resolved Hide resolved

kent-mcleod reviewed Oct 26, 2022

View reviewed changes

components/VM_Arm/src/modules/init_ram.c Outdated Show resolved Hide resolved

chrisguikema force-pushed the multiple_os branch 2 times, most recently from 3257659 to 9f72dee Compare October 27, 2022 00:40

axel-h reviewed Oct 27, 2022

View reviewed changes

components/VM_Arm/configurations/vm.h Show resolved Hide resolved

axel-h reviewed Nov 29, 2022

View reviewed changes

components/VM_Arm/configurations/vm.h Outdated Show resolved Hide resolved

chrisguikema force-pushed the multiple_os branch from 9f72dee to 21812fe Compare January 16, 2023 21:39