-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vm-arm: inline memory initializtion module #68
base: master
Are you sure you want to change the base?
Conversation
+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 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. |
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 |
@hlyytine What do you think about this implementation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me.
Is your logic for removing the init_ram module just that its a small function? In my mind, modules should be for optional things. So things like device tree generation, virtio devices, etc. Basically you want to end up with as small of a VMM as possible, right? Since RAM initialization isn't optional (you need it to load in the guest images), I'm okay with removing it as a module.
components/VM_Arm/src/main.c
Outdated
* of guest memory, e.g. memory shared with other VM and memory that is | ||
* mapped on demand. | ||
*/ | ||
if (ram_size > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ram_size is 0 or less than zero, we have a problem. I'd change this to:
ZF_LOGF_IF(0 > ram_size, "ram_size must be a positive value, as running a guest without RAM is impossible");
err = vm_ram_register_at(&vm, ram_base, ram_size, vm.mem.map_one_to_one);
assert(!err);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ram_size
is an unsigned long
, so this can never be less than zero. With #63 it will become size_t
, which also can't ever get negative. We have zero to disable it now. Allowing negative values here has no use case and it seem to just create more trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point about the negative, but this would still mean a value of 0 is possible, when it would cause a host of issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me.
Is your logic for removing the init_ram module just that its a small function? In my mind, modules should be for optional things. So things like device tree generation, virtio devices, etc. Basically you want to end up with as small of a VMM as possible, right? Since RAM initialization isn't optional (you need it to load in the guest images), I'm okay with removing it as a module.
The first part I agree. For example, we have need to customize FDT. I was thinking perhaps we could have DEFINE_MODULE_EXT
which allows us to specify hooks, for example, during the device tree generation phase. The second that comes to my mind that the fault handler should be extensible with modules. Of course using the MMIO mapping functions is an option, but not sure if we need hooks for unhandled_mem_fault_callback()
(that's how we have been doing it, but not sure if it's 100% necessary).
I was going to say to I disagree with the second part, but but... @axel-h how come could we run our "map guest RAM from dataport" before image loading is done? Perhaps hook or weak function is needed after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some thoughts about modules... I think in more complex systems they will have dependencies so that hooks from module B must be ran before ones from module A. This extended version of DEFINE_MODULE
macro should handle those as well. Perhaps it needs a little more thinking and solving this with weak function is best for short term solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving 0 has always been possible and I don't really know the use case that my change might break. The observable difference now is:
- the APIs are not called at all. But they are not supposed to do anything observable in this case.
- the memory node is not created at all in the DTB. I consider this more sane than creating a node with a zero size. But in the end I'm collecting input here and would add this in a comment then why we need such a node in any case.
It seems @hlyytine had a usecase where 0 was used, because memory came from other places any maybe the images also. I don't think the VMM should try to be extra smart to catch insane configuration. It just have a sane error handling, so things will fail gracefully eventually. But I'm open for suggestion and interested in all special use cases out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say to I disagree with the second part, but but... @axel-h how come could we run our "map guest RAM
from dataport" before image loading is done? Perhaps hook or weak function is needed after all?
@hlyytine Not sure I get the question. Modules are initialized before images are loaded, so they can provide memory that the modules are put into then. This will not change with this PR. It's just that this RAM init now runs before any module runs. So I don't think a weak hook needs to be provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say to I disagree with the second part, but but... @axel-h how come could we run our "map guest RAM
from dataport" before image loading is done? Perhaps hook or weak function is needed after all?@hlyytine Not sure I get the question. Modules are initialized before images are loaded, so they can provide memory that the modules are put into then. This will not change with this PR. It's just that this RAM init now runs before any module runs. So I don't think a weak hook needs to be provided.
OK, that seems to be case. No need for weak function for that reason and I'm fine with that (provided that you give us a way to generate memory node to DTB). But how about backwards compatibility? That init_ram_module() function has been declared weak since ARM VMM got merged into camkes-vm repo and I wouldn't be surprised if somebody somewhere has overridden it. Shall we summon @kent-mcleod to give his opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the memory node is not created at all in the DTB. I consider this more sane than creating a node with a zero size. But in the end I'm collecting input here and would add this in a comment then why we need such a node in any case.
In our use case (map guest RAM from dataport) we need memory node in DTB. We are using autogenerated DTB, so just statically providing it is not enough for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our use case (map guest RAM from dataport) we need memory node in DTB. We are using autogenerated DTB, so just statically providing it is not enough for us.
@hlyytine With the changes in this PR, no "ordinary" memory node is created. Your module would have to create one for the memory it adds. However, this seem impossible in the current code, but I have another PR coming that that allows functions and modules to manipulate the generated DTB according to their "contributions" to the VM. Seems this is exactly what you need then.
Looks good to me now. |
90cd0d1
to
935e312
Compare
@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 |
3d551e8
to
29f1e5c
Compare
Sorry, too many changes for me to understand. Can you look at #70 and tell me what you think. Also I'm not sure if we should do those |
I will move the other commits in this PR to a separate one (see #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. |
I got a bit hesitant about using The second problem is using 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 The fourth problem, we have some code (inside seL4 userland libraries) that uses 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 (
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 Optionally, we can add Jinja2 code that checks the Some reasoning about guest RAM: some embedded operating systems don't need DTB and hence we have CAmkES attributes Just a teaser: Therefore, moving DTB generation to module is the most logical conclusion. With Jinja2, we can translate Lastly, how and when shall we set
My opinion concluded: This PR should put on hold, since IMO it really does not improve anything. I suggest that we discuss whether that 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 Then add |
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 This change would need moving 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 |
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 |
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. |
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 |
74aa768
to
daf56ae
Compare
components/VM_Arm/src/main.c
Outdated
*/ | ||
WEAK int init_ram_module(vm_t *vm) | ||
{ | ||
if (0 == ram_size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the motivation writing comparison in this way, I think it's best to follow the style of existing code. Understanding code is often challenging enough even without mixed style.
vm->mem.map_one_to_one); | ||
if (err) { | ||
ZF_LOGE("RAM registration failed (%d)", err); | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about returning err
instead of plain -1 where possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we have global error codes that each caller can make sense on. So that is why I think it is better if each specific function prints the error and then returns a generic error to the caller, as the caller may not make much sende of the specific error code. But in non-debug builds the error code would get lost then, so you have a point there, that we should change this in the whole file to ensure error codes bubble up and there is a change to have them in non-debug builds also at the top level.
components/VM_Arm/src/main.c
Outdated
@@ -633,7 +656,17 @@ int install_vm_devices(vm_t *vm) | |||
/* Install virtual devices */ | |||
if (config_set(CONFIG_VM_PCI_SUPPORT)) { | |||
err = vm_install_vpci(vm, io_ports, pci); | |||
assert(!err); | |||
if (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this was included accidentally; anyhow, this is not part of inlining init_ram_module()
.
components/VM_Arm/src/main.c
Outdated
} | ||
|
||
/* Install and initialize guest RAM. */ | ||
err = init_ram_module(vm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained above, cookie
parameter is missing. I think it is enough to pass NULL here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you can't really check the return value here since overloaded versions do not return any.
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. |
The trouble seems to be the module here is unnecessary. Weak attribute is the one doing the job. The other alternative is to make Personally I would prefer per-VM modules and |
@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, |
84e7fab
to
7990bee
Compare
8c81335
to
61f4dc8
Compare
{ | ||
if (0 == ram_size) { | ||
ZF_LOGW("VM%d: no RAM defined", vm->vm_id); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to put this check to templates/seL4VMParameters.template.c
? Putting it in this function and having the option to overload the function would lead someone to think that non-zero RAM size is important only if the RAM is mapped in this way. But we already agree no matter how and where the RAM is mapped from, it needs to be non-zero in size?
And I have to nag about style issue once again. Could you use vm_config->ram.size == 0
or !vm_config->ram.size
instead of this, albeit technically superior but not traditional way to write the comparison? If you want the project to use this style, could you start a discussion on seL4 discourse for example and see whether there's support for this in the community? Up until then I think we should not deviate from the existing style.
* custom initialization, e.g. to use memory shared with other components or | ||
* mapped on demand. | ||
*/ | ||
WEAK int vm_init_ram(vm_t *vm, const vm_config_t *vm_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you renamed init_ram_module()
to vm_init_ram()
. This means at least vm_introspect
example in https://github.com/seL4/camkes-vm-examples gets broken (it is broken currently for other reasons, but that's another story). Perhaps some code we don't know of.
Personally I'm not against breaking the ABI but others might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The different name is intentional, it's based on you last comment that changing the signature of an existing function is not a good idea. It this PR gets merged, I will create an issue for vm_introspect
so this gets adapted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[
vm_introspect
] is broken currently for other reasons, but that's another story). Perhaps some code we don't know of.
I think seL4/camkes-vm-examples#39 will fix the brokenness you are referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spotting but not exactly: at some point CAmkES started using as large as possible frames for dataports. Hence if you had more than 2 MB dataport, it uses ARM large pages. On the other hand, currently VMM always uses 4 kB pages in vm_ram_touch()
. To be able to use 2 MB pages from dataport as guest RAM, we use this kind of hack in tiiuae/seL4_projects_libs@fa99679.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to add that you need to use large pages here as well: https://github.com/seL4/camkes-vm-examples/blob/master/apps/Arm/vm_introspect/src/init_dataport_ram.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hlyytine: Could you create an issue in https://github.com/seL4/camkes-vm-examples to track this?
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 |
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. |
b70dafc
to
da09585
Compare
8d610df
to
ef3800a
Compare
adf2e5a
to
63fc28e
Compare
c629f4b
to
65971eb
Compare
- inline code from the module to simplify VMM. - define a weak function vm_init_ram() instead. - improve comments. Signed-off-by: Axel Heider <[email protected]>
Test with: axel-h/camkes-vm-examples#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.