vm-arm: inline memory initializtion module

[axel-h]

Test with: https://github.com/axel-h/camkes-vm-examples/pull/8

Is there a reason why init_ram.c exists as a module. To me this seems overly complicated in the current implementation and I do not really understand the benefit for RAM, as it is essential and not optional.

[hlyytine]

+1 for removing the init_ram module. But can we make it a weak function, so that VMs could use custom code for overriding how the RAM is mapped?

If you happen to remember our presentation at seL4 summit 2022, we are experimenting with providing virtio backends by Linux application (QEMU, crosvm, you name it) running in another VM. Our first prototype was sharing guest memory to this another VM and in order to do that, we used weak attribute of init_ram_module() and provided our own version, which mapped guest physical addresses to dataport frames.

Now I know sharing the whole guest memory is inacceptable in seL4 world and principle of least authority and we are working to share only needed pages with SMMU (work in progress) or with software bounce buffers like Linux SWIOTLB (which we have already working). But we would like to maintain this insecure "share all" version as well, to be able to benchmark how much SMMU/SWIOTLB brings overhead.

I guess mapping RAM from dataport frames is not desirable / wanted to be merged into CAmkES-VM as such, but please provide facilities to be able to override RAM initialization. Weak symbol is the easiest.

[axel-h]

I have seen that we were experimenting with this in you fork. What I can do is wrap this in a internal helper function that is weak. What we could als do it on call vm_ram_register_at() if ram_size is not zero, ie. if there is any exclusive VM RAM. In your case there would be another class of RAM then that can be added via the module hooks, which is just configures in the CMake file then. So there is no need for any weak functions.

[axel-h]

@hlyytine What do you think about this implementation?

[hlyytine]

@hlyytine What do you think about this implementation?

Looks good to me now.

[axel-h]

@hlyytine I've added a commit that makes the DTB generation more dynamic, so modules can also add data to it. Would this allow you to implement youe use case, if you generate a memory node when ram_size is 0?

[hlyytine]

@hlyytine I've added a commit that makes the DTB generation more dynamic, so modules can also add data to it. Would this allow you to implement youe use case, if you generate a memory node when ram_size is 0?

Sorry, too many changes for me to understand. Can you look at seL4/camkes-vm#70 and tell me what you think.

Also I'm not sure if we should do those if (generate_dtb) things, perhaps we can also load DTB first (provide_dtb=1) and then customize it (generate_dtb=1), that way there would be no need for dtb_base_name. Also let DTB loading fail gracefully if generate_dtb=1, in that case rely on completely autogenerated DTB. I guess the latter originated from Dornerworks, so what do you @chrisguikema think, can we imagine simplifying DTB loading code flow like this?

[axel-h]

I will move the other commits in this PR to a separate one (see https://github.com/seL4/camkes-vm/pull/73). Creating the DTB unconditionally is an option, but I think it's not so nice as it does unnecessary work any may cause other issues.

[hlyytine]

I got a bit hesitant about using ram_size == 0 notion to disable init_ram module functionality. I think that even we, those familiar with the code, should not rely on our memory. I mean, after successful summer vacation and empty mind, it should be enough to deduce that ram_size variable contains the amount of guest RAM. We should not need to remember that ram_size == 0 means that it does not mean guest does not have any RAM, but rather it is defined somewhere else. Someone might think the guest is emulating XIP ROM and we don't want to steepen seL4 world learning curve, do we?

The second problem is using ram_size == 0 forces alternative means to introduce yet another configuration variable into CAmkES file. For example in our use case, ram_size is the authoritative figure and we just use a CAmkES dataport that is able to provide enough frames to satisfy stage-2 mapping code. It's also kind of self documenting when we set explicitly both ram_size and dataport's size to VMx_RAM_SIZE.

The third problem, or actually an extension of the second, is that seems that you want to get rid of global variables. Good. I think the reason why we want that is to have pure functions. In other words, to get rid of side effects. Now, enabling or disabling some unknown code based on whether ram_size is zero or not does not seem particularly good idea to me. The least we should do is to print a warning message "RAM size is defined as zero, expect some undefined behaviour.

The fourth problem, we have some code (inside seL4 userland libraries) that uses ram_size variable. I guess it is not unreasonable to expect somebody else does the same thing somewhere. Now, are we expected to create a new variable for our differently mapped RAM size and patch all the libraries accordingly? Or shall we assign ram_size ourselves in our "use CAmkES dataport as RAM" code? If yes to latter, how to communicate this needed fix to all those to might have custom RAM mapping code?

Hence I have a proposition, and this time I will try to explain my thoughts in English instead of PR, but let me address another concern first.

I see you didn't particularly like having multiple module API functions (init() and fdt_generate() in seL4/camkes-vm#70) and I think you have a point there. I'm fine if we stick with having only init_module() entry point, at least for now. But what bothers me is the void return type. The only way to react to errors is assert() and you cannot propagate the error up the stack to the caller.

1. Leave it as is.
2. Leave it as is and return to this problem later.
3. Introduce an alternative init() function with int return type and use it if defined, otherwise use the old-style init_module() function (in the VMM module loading code). In that case, print warnings also.
4. Add enum module_state { UNINITIALIZED=0, INITIALIZING=1, FAILED=2, INITIALIZED_IF_RETURNS=3, INITIALIZED=3 } and add the status field to each struct vmm_module. Now before actually calling init_module(), module loading code sets state to INITIALIZED_IF_RETURNS. With this we kind of prepare for future when we have proper int-returning initializer.
5. Combine 3 and 4.

My personal preference at the moment is 2, followed by 5.

Now back to original problem. The first question is how to allow custom RAM mapping code. I propose that we add a VM attribute modules to CAmkES file, defaulting to all to retain backwards compatibility. The whitespace separated strings could be prefixed with - (remove), otherwise implicit + (add) is assumed. Hence -all and empty string are equal, no modules loaded (just a theoretical example). More practical example in our use case could be -init_ram, our custom module would be included in that "all but init_ram" set. If you need to make sure your VMM has only init_ram module, use "-all init_ram". But like I mentioned, if you don't want to be bothered with this, just ignore this, the default all makes everything work like earlier.

Optionally, we can add Jinja2 code that checks the modules VM attribute and in case some module is explicitly requested in that string but disabled at CMake level, we can issue a warning or halt the build.

Some reasoning about guest RAM: some embedded operating systems don't need DTB and hence we have CAmkES attributes generate_dtb and provide_dtb. But at hand-written C code level, adding RAM to guest and generating DTB for guest are completely (almost) orthogonal things. The only corner case is that guest does not have any RAM, but even in deeply embedded system with XIP program code in ROM, there is small SRAM for working set. Hence we can drop the almost and say they are completely orthogonal things. So if we are generating DTB, we need to create that memory node also in all cases, regardless whether the RAM is mapped to guest using this code or some custom code. Hence it is reasonable to defend the POV that ram_base and ram_size global variables could and should exist after some certain of execution flow (installing VMM modules in this case).

Just a teaser: Therefore, moving DTB generation to module is the most logical conclusion. With Jinja2, we can translate "generate_dtb" : false to -generate_dtb to be appended to modules. Think about seL4 microkernel. It does the bare minimum only, the rest is done in userspace threads. Why should the VMMs on top of it be of any different design? Shouldn't it just do the bare minimum and delegate the rest to modules (analogy to userspace threads?)

Lastly, how and when shall we set ram_base and ram_size. We know that after loading VMM modules they are guaranteed to have sane values and for the remainder of the code, we can take it granted. But where do these values originate? I would imagine that ultimately they are a couple of #define VMx_RAM_{BASE,SIZE} just like before but since guest RAM seems to be a required component of a CAmkES VMM, should this configuration be handled through CAmkES interface some day. Then you hook your VM with a CAmkES component that does the same thing as init_ram does today, or we at SSRC could hook the VM to a RAM-providing component that is really an extended version of seL4SharedCapsWithData. Then my opinions about this:

• Overengineered? Perhaps. But why do we one rather well thought of component system and then additional something more or less hackish one.
• Shall we start implementing this now? No.
• Shall we start coding this in near future? I don't see immediate need.

My opinion concluded: This PR should put on hold, since IMO it really does not improve anything. I suggest that we discuss whether that modules VM attribute approach is more suitable.

Action point: Start discussion on VMM seL4 discourse.

Still one more thought: we really should not compile in code that is not used. Hence perhaps this is best handled at build time at CMake level. This way we could have init_ram for one VM and our customized one for another VM. Also we can use linker, C preprocessor and Jinja2 to deduce at build time that we have all required modules and we don't have clashing modules. Example of this would be #define MODULE_PROVIDES(__x) const int VMM_MODULE_PROVIDES_ ##__x = 0 and then having MODULE_PROVIDES(ram) in both modules. Trying to enable them at the same time will not even link.

Then add #define MODULE_REQUIRES(__x) { extern const int VMM_MODULE_PROVIDES_ ##__x; volatile int __require = VMM_MODULE_PROVIDES_##__x; } and add MODULE_REQUIRES(ram) to make sure some guest RAM providing module is loaded.

[hlyytine]

I got even simpler idea: seems that we all agree RAM is needed from VM in any case, which means linker must be able to resolve ram_base and ram_size. Wouldn't it be the most logical place for these global variables to be where the RAM mapping is done, in this case modules/init_ram.c? Or in our SSRC case, in our alternative module. That way you would be forced to provide exactly one implementation of RAM mapping code per VM. Provide none, and linker fails due to missing symbol. Provide more than one, and linker fails due to multiple definitions of symbols.

This change would need moving ram_base and ram_size (and backwards compatibility linux_ram_{base,size}) from templates/seL4VMParameters.template.h to a private header templates/seL4GuestVMRAMConfiguration.h, which is included by RAM mapping modules only.

We would still need a mechanism to turn off init_ram module and add ours, but other than that, no other changes needed at all.

Edit 13:00 Finnish time: I forgot there are no private headers in CAmkES templates. Hence we must guard the contents of templates/seL4VMGuestRAMConfiguration.h with #ifdef that is defined only in init_ram.c (or similar code).

[kent-mcleod]

I guess mapping RAM from dataport frames is not desirable / wanted to be merged into CAmkES-VM as such, but please provide facilities to be able to override RAM initialization. Weak symbol is the easiest.

I think this was actually why the init_ram module was made. This example uses a ram dataport to introspect a guest VMs ram device: https://github.com/seL4/camkes-vm-examples/blob/master/apps/Arm/vm_introspect/src/init_dataport_ram.c#L39-L45

[kent-mcleod]

Still one more thought: we really should not compile in code that is not used. Hence perhaps this is best handled at build time at CMake level.

I recall that this simple module system was added to allow general code to be configured in CMake to be compiled in/out based on build time configuration and to also support camkes connectors that use templates that define modules and can thus be initialized at start of component runtime.

I also am not sure about the motivation of this PR is as the module was added for the vm_introspect example to perform its own ram installation.

[axel-h]

Thanks. I get the point why this should be a weak function. But I still seems a bit odd to me that this is in a module. It would be a bit more intuitive if the weak function is in main.c then with a comment explaining that this is a explicit customization point for modules.

[hlyytine]

I think this was actually why the init_ram module was made. This example uses a ram dataport to introspect a guest VMs ram device: https://github.com/seL4/camkes-vm-examples/blob/master/apps/Arm/vm_introspect/src/init_dataport_ram.c#L39-L45

Thanks for reminding. Funny enough, that's where I started learning how to share memory between VMs but seems that I forgot it completely. On a more serious note, since there are at least two uses now (vm-introspect and our virtio stuff) one potential idea is to consolidate "RAM as dataport" to a VMM module, but before that, two things need to be solved: a) per-VM configuration and b) how to tell it which dataport to use.

[hlyytine]

I also am not sure about the motivation of this PR is as the module was added for the vm_introspect example to perform its own ram installation.

The trouble seems to be the module here is unnecessary. Weak attribute is the one doing the job. The other alternative is to make init_ram a per-VM configurable item, then weak function would not be needed but it would be a API breaking change. Perhaps API is too strong a word, though. If it's not documented, it's not an API, is it?

Personally I would prefer per-VM modules and init_ram reworked somehow along the lines I have presented somewhere in the comments above. But the other altenative is to remove the confusing element, the module, as this PR suggests.

[hlyytine]

@axel-h IMO inlining this would be good, as the weak function kind of negates the purpose of the module system. But how about if you just copied & pasted that function without making any non-whitespace changes. Because it is in the same repo, git diff with coloring turned on would show it clearly just as moved code. Then the commit message would explain why the module system does not bring any additional benefits here. If you really insist doing other changes, they could be an another PR / commit.

[hlyytine]

To be honest, I think this PR creates more problems than it solves.

I know I repeat myself, but I really believe VMM design should be as minimal as it could be and consolidating the module functionality into main.c actually does the opposite.

[axel-h]

To be honest, I think this PR creates more problems than it solves.

I think this is similar to vPCI, there I also failed for make it purely module because a certain awareness seems a core part of the VMM. As long as we don't have more flexible module API and a proper dependency handling, avoiding hacky modules appears a better was forward for a cleaner VMM.