muslcsys: implement dummy munmap call

[chrisguikema]

This commit adds a dummy __NR_munmap call, which prevents the "libsel4muslcsys: Error attempting syscall 215" error from printing.

That print shows up during the ARM VMM boot process.

[chrisguikema]

Apparently the style checker didn't like my attempted fixes, so I'm just going to leave it alone for now

[kent-mcleod]

That print shows up during the ARM VMM boot process.

Is this safe to ignore? My guess is that the vmm does a large malloc that internally uses mmap, and then when the memory is freed, munmap is called. The sel4_libs, mmap implementation doesn't support freeing (it's a bump allocator) and so expects that munmap is never called.

[axel-h]

Apparently the style checker didn't like my attempted fixes, so I'm just going to leave it alone for now

I've also seen such strange complaints happening with this library, but so far could not figure out what exactly causes this.

[kent-mcleod]

I've also seen such strange complaints happening with this library, but so far could not figure out what exactly causes this.

Yea, the style checker has a bug with that line where it always thinks the indentation is wrong no matter how it's set.

[chrisguikema]

My guess is that the vmm does a large malloc that internally uses malloc

You're right. load_image malloc's a 1MB buffer (BIT(20)). I tested and dropping down to BIT(17) removes the libsel4muslcsys error.

What's the path forward here? Should be make a PR in seL4_projects_libs to drop the load_image buffer size? Pull a buffer from the stack?

Or should I update the commit to reflect Axel's wording? Or should we do both?

[chrisguikema]

Yea, the style checker has a bug with that line where it always thinks the indentation is wrong no matter how it's set.

That seems like an odd bug, but good to know!

[kent-mcleod]

What's the path forward here? Should be make a PR in seL4_projects_libs to drop the load_image buffer size? Pull a buffer from the stack?

Or should I update the commit to reflect Axel's wording? Or should we do both?

I think it's still an improvement to implement this syscall, but with a ZF_LOGE so that it always prints the error. This would be an improvement over printing a syscall number.

As for the callsite, do you think it's possible to refactor guest_write_address and load_image to not need the malloc call at all? If we reorder the calls so that the read happens inside the vm_ram_touch callback, then it can write the image straight into the guest ram memory instead.

[chrisguikema]

As for the callsite, do you think it's possible to refactor guest_write_address and load_image to not need the malloc call at all?

Let me check. I like that thought process.

[chrisguikema]

@kent-mcleod I was able to do the read from the vm_ram_touch callback. That has the basic effect of reading the file from the server 1 page at a time, but it didn't seem too much slower to me. vm_ram_touch basically does the memcpy 1 page at a time anyways. Might be worth a benchmark to see the performance hit of this method.

    size_t file_size = get_file_size(image_name);

    fd[0] = open(image_name, 0);
    if (fd[0] == -1) {
        ZF_LOGE("Error: Unable to find image \'%s\'", image_name);
        return -1;
    }

    size_t read_size = PAGE_SIZE_4K;

    size_t offset = 0;
    while (offset < file_size) {
        vm_ram_mark_allocated(vm, load_addr + offset, read_size);
        len = vm_ram_touch(vm, load_addr + offset, read_size, guest_write_address, (void *)fd);
        if (-1 == len) {
            ZF_LOGE("Error: Failed to load \'%s\'", image_name);
            close(fd[0]);
            return -1;
        }
        offset += len;
    }
    *resulting_image_size = offset;
    close(fd[0]);
    return 0;

[kent-mcleod]

One page at a time should be fine if the page sizes are large enough.

[lsf37]

Yea, the style checker has a bug with that line where it always thinks the indentation is wrong no matter how it's set.

That seems like an odd bug, but good to know!

I'll have a look at that, maybe there is something we can tweak so we don't trigger it at least.

[lsf37]

I've added a commit that avoids triggering the bug. (The bug seems to be that astyle always adds another indent for the second line, no matter where it is. If the line is its own comment that doesn't happen).

[chrisguikema]

Thanks for looking into that error, Gerwin! I never like when the CI checks fail, even if its not something that the PR specifically called

[lsf37]

I never like when the CI checks fail, even if its not something that the PR specifically called

Same here :-)