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.

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

feat(vm): change UhyveVm struct parameters #755

Closed
wants to merge 1 commit into from

Conversation

n0toose
Copy link
Member

@n0toose n0toose commented Sep 18, 2024

As I was moving some functionality away from new(...) so as to progress with my work on ASLR and some future work, I found it necessary to use certain parameters later. A particular example from UhyveVm's current structure would be params.thp and params.ksm, which pose one of the obstacles preventing us from initializing the memory later (e.g. in load_kernel or init_guest_mem, after loading the kernel and being able to establish a guest address), even though separate variables for those don't make much sense.

This change will mostly be useful for future work, but aims to establish a consistent convention now.

@n0toose
Copy link
Member Author

n0toose commented Sep 18, 2024

P.S. This change was made before the latest changes to #725 - it seems like I forgot to push it before, but it still makes somewhat sense to me even after the changes. I'll gladly take care of the rebasing for the other PR (as well as explain some more examples via teleconference tomorrow) :)

It's one of those "architectural changes" that I'm unsure about. I'll fix up the clippy warnings and clean things up, but I'd be interested in an opinion.

@n0toose n0toose force-pushed the change-struct-params branch 4 times, most recently from 9749700 to d0775a8 Compare September 18, 2024 22:49
As I was moving some functionality away from new(...) so as to progress
with my work on ASLR and some future work, I found it necessary to use
certain parameters later. A particular example from UhyveVm's current
structure would be params.thp and params.ksm, which pose one of the
obstacles preventing us from initializing the memory later (e.g. in
load_kernel or init_guest_mem, after loading the kernel and being able
to establish a guest address), even though separate variables for those
don't make much sense.

This change will mostly be useful for future work, but aims to
establish a consistent convention now.
Copy link
Member Author

@n0toose n0toose left a comment

Choose a reason for hiding this comment

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

I made quite a few decisions that I wasn't very sure about because I felt that they'd bring me to the right direction. Happy to explain.

@@ -105,19 +104,24 @@ pub struct UhyveVm<VCpuType: VirtualCPU = VcpuDefault> {
entry_point: u64,
stack_address: u64,
pub mem: Arc<MmapMemory>,
pub params: Params,
memory_size: usize,
Copy link
Member Author

Choose a reason for hiding this comment

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

memory_size might be useful if we move the memory initialization outside of new(...) (perhaps using a MaybeUninit - not quite sure).

Copy link
Member

Choose a reason for hiding this comment

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

But you can access the memory_size via mem already?

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 wasn't exactly sure whether doing this would get in the way of your work, given such a way to access this variable won't be possible after the replacement of the MmapMemory interface in #643.

There is some technical debt in #725 which does the exact same thing, but, as a trade-off, I assumed that a superfluous field would've been better than increasing that technical debt.

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.26%. Comparing base (5c9a2fd) to head (49b012c).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/vm.rs 63.63% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
+ Coverage   67.24%   67.26%   +0.01%     
==========================================
  Files          20       20              
  Lines        2351     2346       -5     
==========================================
- Hits         1581     1578       -3     
+ Misses        770      768       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jounathaen
Copy link
Member

It is a bit unfortunate, that we are simultaneously working on this bit of code.
So, I appreciate the work here, and I like some parts of it, but I tend to reject this one. I just think there is not too much value added by adding params to vm.
Give me some more time to finalize #725 and then we'll see how much of this kind of changes remain necessary.

@jounathaen jounathaen closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants