Skip to content
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.

Sign up for GitHub

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

Jump to bottom

vm-arm: inline memory initializtion module #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

vm-arm: inline memory initializtion module #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion arm_vm_helpers.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ function(DeclareCAmkESARMVM init_component)
${ARM_VM_PROJECT_DIR}/components/VM_Arm/src/fdt_manipulation.c
${ARM_VM_PROJECT_DIR}/components/VM_Arm/src/crossvm.c
${ARM_VM_PROJECT_DIR}/components/VM_Arm/src/modules/map_frame_hack.c
${ARM_VM_PROJECT_DIR}/components/VM_Arm/src/modules/init_ram.c
)

if(VmVirtUart)
Expand Down
32 changes: 32 additions & 0 deletions components/VM_Arm/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,31 @@ static void irq_handler(void *data, ps_irq_acknowledge_fn_t acknowledge_fn, void
}
}

/* Default RAM initialization. Modules can overwrite this weak function with a
* 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)
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

@hlyytine hlyytine Apr 13, 2023 •
edited
Loading

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.

Copy link
Contributor

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

Copy link
Member Author

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?

{
/* A VM without RAM is highly unusual and unlikely to work. But there might
* be special VM configurations where modules create specific RAM areas. In
* this case, this weak RAM init function here should be overwritten also,
* that's why we print the warning.
*/
if (0 == vm_config->ram.size) {
ZF_LOGW("no standard RAM defined");
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under illusion that we agreed that all systems have some RAM, even if very small amount. Now this 0 seems to contradict that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've kept this with the comment as sanity check. But I can remove it also completely.

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will create a PR that adds a sanity check in templates/seL4VMParameters.template.c and don't print anything here.

}

int err = vm_ram_register_at(vm, vm_config->ram.base, vm_config->ram.size,
vm->mem.map_one_to_one);
if (err) {
ZF_LOGE("RAM registration failed (%d)", err);
axel-h marked this conversation as resolved.
Show resolved Hide resolved
return -1;
Copy link
Contributor

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?

Copy link
Member Author

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.

}

return 0;
}

/* Force the _vmm_module section to be created even if no modules are defined. */
static USED SECTION("_vmm_module") struct {} dummy_module;
Expand All @@ -689,6 +714,13 @@ static int install_vm_devices(vm_t *vm, const vm_config_t *vm_config)
{
int err;

/* Initialize guest RAM. */
err = vm_init_ram(vm, vm_config);
if (err) {
ZF_LOGE("Failed to initialize RAM (%d)", err);
return -1;
}

/* Install virtual devices */
if (config_set(CONFIG_VM_PCI_SUPPORT)) {
err = vm_install_vpci(vm, io_ports, pci);
Expand Down
24 changes: 0 additions & 24 deletions components/VM_Arm/src/modules/init_ram.c
View file
Open in desktop

This file was deleted.