mmap_unix: add support for externally managed mappings

[slp]

Add the unsafe MmapRegion::build_raw method to allow users to instance
MmapRegion with externally managed mappings. Add also a field to
indicate whether the mapping is internal or external, so we don't try
to munmap it on MmapRegion::Drop.

Signed-off-by: Sergio Lopez [email protected]

[alexandruag]

Hi! If possible, can you please share more details regarding the use case(s) for this change? Some of the usage examples I'm trying to think of can be accomplished via alternative means (for example, mapping the same memfd in multiple places).

[slp]

Hi! If possible, can you please share more details regarding the use case(s) for this change? Some of the usage examples I'm trying to think of can be accomplished via alternative means (for example, mapping the same memfd in multiple places).

@alexandruag In libkrun we need to create a MmapRegion for a section coming from a dynamically linked library that has been mapped by the linker. The file backing the library may no longer be accessible due to context changes (namespace manipulation).

I acknowledge that our use case may be a bit particular, but I think the change we need in vm-memory is so small and simple that shouldn't become a maintenance burden.

[alexandruag]

Thanks for the explanation! libkrun looks very interesting! Just left a minor comment, and it seems you also need to update the coverage value in coverage_config_x86_64 (more details on the CI results page).

[alexandruag]

owned might be a bit more suggestive here (+ reversing the significance of true/false), WDYT?

[slp]

Sounds good to me.

[slp]

Thanks for the explanation! libkrun looks very interesting! Just left a minor comment, and it seems you also need to update the coverage value in coverage_config_x86_64 (more details on the CI results page).

@alexandruag Thanks! I've updated the PR to replace external with owned and fix the coverage value (wasn't sure who should update it, thanks for the clarification).

[alexandruag]

Looks good! I just realized there are two points I forgot to mention in my previous comment, sorry about that :(

While I don't think this is mentioned anywhere yet, we do effectively rely on regions having certain alignment and size restrictions. I guess it will be simplest to state at some point (and check) that a region is aligned to a page boundary and its size is a multiple of the page size. Does that hamper your use case in any way?

Also, I forgot we have a changelog now :-s Can you please add an Added entry in CHANGELOG.md for the [Unreleased] version?

[jiangliu]

Looks good! I just realized there are two points I forgot to mention in my previous comment, sorry about that :(

While I don't think this is mentioned anywhere yet, we do effectively rely on regions having certain alignment and size restrictions. I guess it will be simplest to state at some point (and check) that a region is aligned to a page boundary and its size is a multiple of the page size. Does that hamper your use case in any way?

We do have some implicit assumption that the base address of a region is PAGE aligned. And on Linux, the mmap() syscall ensures that file offset and returned address are page aligned. Not sure about the behavior mmap on Windows.
So we should ensure the region base and length are PAGE aligned.

[slp]

While I don't think this is mentioned anywhere yet, we do effectively rely on regions having certain alignment and size restrictions. I guess it will be simplest to state at some point (and check) that a region is aligned to a page boundary and its size is a multiple of the page size. Does that hamper your use case in any way?

Sounds good, and shouldn't be a problem for libkrun. I've just added those checks and extended the test to ensure it also fails when expected. Could you please take a look?

Also, I forgot we have a changelog now :-s Can you please add an Added entry in CHANGELOG.md for the [Unreleased] version?

Sure, no problem. I see all the existing entries refer to an issue, do I need to create one covering the goal of this PR?

[slp]

Hm... coverage has dropped by 0.30% now... will take a look at it tomorrow.

[slp]

Hm... coverage has dropped by 0.30% now... will take a look at it tomorrow.

Okay, so the coverage drop was caused by the newly added Errors, so I've added a test_display_error test and coverage is now up to 86.0% (from 85.2%).

[alexandruag]

Thanks for the changes! Things look good. We'll document the alignment & such requirements after we polish the interface a bit more, and by then we should also figure out what the exact model & semantics are (regions being aligned looks mandatory, but the others might be relaxed).

I think it's fine to use a reference to the PR itself in the change log as well, but just in case this contradicts some style guidelines we're trying to establish, do you mind if we wait just a bit more to see if others have a different opinion?

[slp]

I think it's fine to use a reference to the PR itself in the change log as well, but just in case this contradicts some style guidelines we're trying to establish, do you mind if we wait just a bit more to see if others have a different opinion?

No problem, we can wait a bit to see what's the consensus about that. Thanks!

[andreeaflorescu]

Regarding the changelog, since there is no associated issue with this PR, we can either create one, or link the PR. As @alexandruag said, we don';t really have any policy regarding this yet.

Also, sorry for jumping in with the review so late. Feel free to consider my comments nits and address them in a subsequent PR. They are mostly related to documenting the added functionality so that external users have an easier time :D

[slp]

@andreeaflorescu I've rebased the commit and addressed your comments. PTAL, thanks!

[slp]

I've just noticed that I forgot to add the entry to CHANGELOG.md. Fixed now.