-
Notifications
You must be signed in to change notification settings - Fork 14
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
Memory as file descriptor #195
base: master
Are you sure you want to change the base?
Conversation
8639f75
to
ba48425
Compare
4e95742
to
4fad009
Compare
88b48f4
to
59a7ad4
Compare
59a7ad4
to
cd4dc16
Compare
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 14.19% 18.62% +4.43%
==========================================
Files 5 7 +2
Lines 472 671 +199
Branches 88 108 +20
==========================================
+ Hits 67 125 +58
- Misses 383 513 +130
- Partials 22 33 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This PR is ready for review. |
@Wenzel Will do 👍 |
Imho the microvmi struct looks a bit overly complicated. I would prefer a design where you (as a library user) initialize the driver like we used to and then pass it to one of the memory abstraction classes: let drv = driver_init();
let memory = Memory::new(drv);
let padded_memory = Paddedmemory::new(drv); Not every user needs memory abstractions, but those who do could easily initialize them. Edit: Right now this implementation would force the library user to write single threaded applications (because |
Hi @rageagainsthepc and thank you for your feedback. I should have explained a bit more why I decided this implementation and design when opening the PR. The issues comes from exposing this API in C and Python. because how do you keep the context ? The C API exposes a That's the main reason why I choose to go this way, keep a single struct A second reason is that we are able to simplify our read implementations in the drivers, mainly the Xen driver, since the main algorithm to iterate over each frame is implemented in the layer above. And another reason is to be able to implement batch read operation later on, hidden in the Memory object implementation. Edit: as a bonus, I just noticed that with the |
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 would at least improve the tests. As for the other comments: I understand if you are set on your current approach, I'll leave that up to you.
} | ||
|
||
impl PageFrame { | ||
pub fn with_paddr(paddr: u64) -> Self { |
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.
nit: Imho the name is misleading. with_paddr(paddr: u64)
only makes sense if this function would transform an existing PageFrame
. Since it simply creates a new one out of thin air I would stick to new(paddr: u64)
.
} | ||
} | ||
|
||
pub fn to_paddr(&self) -> u64 { |
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.
nit:
pub fn to_paddr(&self) -> u64 { | |
pub fn paddr(&self) -> u64 { |
Probably just a matter of personal taste but it makes more sense to me.
&mut bytes_read_local, | ||
) | ||
.is_ok(); | ||
if driver |
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.
This file was supposed to be a simple C abstraction layer. Augmenting it with code that goes beyond that is bad design imho.
mock_introspectable | ||
.expect_get_max_physical_addr() | ||
.returning(move || Ok(max_addr)); | ||
let mut memory = Memory::new(Rc::new(RefCell::new(Box::new(mock_introspectable)))).unwrap(); |
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 would welcome it if we followed the arrange, act, assert pattern as it makes the tests more readable imo. https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/
I don't think that you should add comments like the article does, but simply adding newlines between the three blocks would be helpful.
.returning(move || Ok(max_addr)); | ||
let mut memory = Memory::new(Rc::new(RefCell::new(Box::new(mock_introspectable)))).unwrap(); | ||
// seek 0 doesn't move position | ||
assert_eq!(0, memory.seek(SeekFrom::Start(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.
Don't put multiple unrelated asserts into the same test case, if you want to avoid code duplication use the test-case
crate. See https://github.com/Wenzel/libmicrovmi/blob/master/src/driver/kvm.rs#L398
/// # Arguments | ||
/// | ||
/// * `domain_name` - The domain name | ||
/// * `driver_type` - The driver type to initialize. None will attempt to initialize every driver avaiable |
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.
/// * `driver_type` - The driver type to initialize. None will attempt to initialize every driver avaiable | |
/// * `driver_type` - The driver type to initialize. None will attempt to initialize every driver available |
} | ||
|
||
/// Retrieve the number of VCPUs. | ||
pub fn get_vcpu_count(&self) -> Result<u16, Box<dyn Error>> { |
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.
You basically implement the Introspectable
trait without officially implementing the Introspectable
trait. That's nasty. :/
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.
It's also a lot of code duplication and unnecessary indirection. What's so wrong about making the drv
public in order to allow direct access?
This PR implements the memory as a file descriptor, using Rust standard traits
Read
Write
Seek
A second memory object is implemented to handle "padded" memory reads, useful for Volatility.
Changes
To introduce this memory abstraction, I had to update the API.
The library doesn't return a
dyn Introspectable
anymore, but aMicrovmi
struct, which contains the driver and both memory objects (padded and non padded).On this
Microvmi
struct, the methods are simply forwarding to the driver implementation.Regarding the memory objects, I had to use a
Rc<Refcell<Box<dyn introspectable>>>
to share the driver between all the structs, allowing a mutable borrow checked at runtime.I tested against master while dumping the memory and the overhead is invisible.
Some unit tests have been implemented for the seek implementation.
I also changed how the drivers should implement reading physical memory.
The new API is:
Which allows us to remove complexity from the driver implementation to the
Read
trait implementation (especially for Xen), and share this for all drivers.