implement KvmVec struct #6

serban300 · 2019-03-29T15:53:52Z

KvmVec provides a common abstraction for all the KVM structures that resemble arrays. For example: CpuId and MsrList

acatangiu

Noice! 👌

src/ioctls/kvm_vec.rs

acatangiu · 2019-03-29T16:30:47Z

src/ioctls/kvm_vec.rs

+    /// It is important to call `mem::forget` after using this vector. Otherwise rust will destroy it.
+    ///
+    unsafe fn as_vec(&mut self) -> Vec<F> {
+        let entries_ptr = self.as_mut_original_struct().entries_mut().as_mut_ptr();


nit: any reason why you're always using the accessor methods instead of just working with the struct members directly?

Suggested change

let entries_ptr = self.as_mut_original_struct().entries_mut().as_mut_ptr();

let entries_ptr = self.kvm_array[0].entries_mut().as_mut_ptr();

I just find it more readable this way. kvm_array[0] looks weird. as_mut_original_struct gives more information on what we're actually accessing.

src/ioctls/kvm_vec.rs

src/ioctls/mod.rs

src/ioctls/kvm_vec.rs

acatangiu · 2019-04-01T13:53:05Z

src/ioctls/kvm_vec.rs

+
+        let required_kvm_array_len = KvmVec::<T>::num_elements_to_kvm_array_len(self.len);
+        unsafe {
+            self.kvm_array.set_len(required_kvm_array_len);


nit: add comments for this function explaining the logic to make it easier to follow

acatangiu

LGTM

src/ioctls/kvm_vec.rs

andreeaflorescu

Please add usage examples for the public interface in doc comments. Some examples in this PR: #13

src/ioctls/kvm_vec.rs

src/ioctls/system.rs

serban300 · 2019-04-09T10:27:43Z

Can't think of meaningfull usage examples that would compile since KvmArray and KvmVec are private

Please add usage examples for the public interface in doc comments. Some examples in this PR: #13

andreeaflorescu · 2019-04-09T14:55:34Z

@serban300 as discussed offline the trait needs to be public so you can access the methods associated with CpuId. If you do some more investigation and you discover that they actually don't need to be public, we can write the usage examples for either CpuId or for MSRs.

serban300 · 2019-04-10T13:39:43Z

@andreeaflorescu I added examples

andreeaflorescu · 2019-04-12T10:26:35Z

src/ioctls/mod.rs

    }
 }

-#[cfg(test)]
+/// Wrapper for `kvm_cpuid2`.


Please add details about CpuId. Right now when you open the documentation of CpuId it doesn't contain any relevant information about how to use the structure:

Type Definition kvm_ioctls::CpuId type CpuId = KvmVec<kvm_cpuid2>; [−] Wrapper for kvm_cpuid2.

I would suggest two things:

In the documentation description add a link to KvmVec. (e.g. CpuId implements trait 1 and trait 2 with methods for getting kvm_entries bla bla. And for the traits it implements you can add them as links to internal documentation. You can see some examples [here])(https://rust-lang-nursery.github.io/api-guidelines/documentation.html#prose-contains-hyperlinks-to-relevant-things-c-link).

Add a minimal usage example for CpuId. You can copy one that already exist in the previous documentation. For example:

/// # Example /// ```rust /// use kvm_ioctls::{CpuId, Kvm, MAX_KVM_CPUID_ENTRIES}; /// let kvm = Kvm::new().unwrap(); /// let mut cpuid = kvm.get_supported_cpuid(MAX_KVM_CPUID_ENTRIES).unwrap(); /// let cpuid_entries = cpuid.mut_entries_slice(); /// ```

Same for MsrList.

andreeaflorescu · 2019-04-12T10:28:41Z

src/ioctls/common/kvm_vec.rs

+    fn entries_mut(&mut self) -> &mut __IncompleteArrayField<Self::Entry>;
+}
+
+/// An adapter that helps in treating a KvmArray more like an actual `Vec`.


"more like an actual" these are all filler words. Please rephrase.

src/ioctls/common/kvm_vec.rs

andreeaflorescu · 2019-04-12T10:31:54Z

src/ioctls/common/kvm_vec.rs

+
+    /// Returns a reference to the actual KVM structure instance.
+    ///
+    pub fn as_original_struct(&self) -> &T {


nit: as_kvm_struct? Same for the others.

Done. What do you mean by "the others" ?

Probably I wanted to write this somewhere else, sorry for the confusion.

andreeaflorescu · 2019-04-12T10:55:18Z

src/ioctls/common/kvm_vec.rs

+
+        self.reserve(1);
+
+        let mut entries = unsafe { self.as_vec() };


Why is this safe? Please add comment before unsafe blocks.

andreeaflorescu · 2019-04-12T11:01:11Z

src/ioctls/common/kvm_vec.rs

+impl<T: Default + KvmArray> KvmVec<T> {
+    /// Returns the capacity required by kvm_array in order to hold the provided number of  `KvmArray::Entry`
+    ///
+    fn num_elements_to_kvm_array_len(num_elements: usize) -> usize {


nit: I would rename this to kvm_vec_capacity. And also add a comment that it returns the kvm vec capacity required to hold num_elements of KvmArray::Entry.

I think kvm_vec_capacity would be confusing. I renamed kvm_array to 'mem_allocator' and I renamed this method to kvm_vec_len_to_mem_allocator_len

andreeaflorescu · 2019-04-12T11:07:08Z

src/ioctls/common/kvm_vec.rs

+        let additional_kvm_array_len = required_kvm_array_len - current_kvm_array_len;
+
+        self.kvm_array.reserve(additional_kvm_array_len);
+        self.capacity = desired_capacity;


The capacity should be updated to self.kvm_array.capacity().

src/ioctls/common/kvm_vec.rs

andreeaflorescu · 2019-04-12T11:23:07Z

src/lib.rs

@@ -42,3 +43,6 @@ pub use ioctls::{KvmRunWrapper, Result};
 /// It can be used for calls to [get_supported_cpuid](struct.Kvm.html#method.get_supported_cpuid) and
 /// [get_emulated_cpuid](struct.Kvm.html#method.get_emulated_cpuid).
 pub const MAX_KVM_CPUID_ENTRIES: usize = 80;
+
+/// Maximum number of MSR entries that can be returned by a call to KVM ioctls.


Please add similar details as the MAX_KVM_CPUID_ENTRIES have. Where did you get the value from? What can it be used for?

I took it fro mthe prefious implementation of get_msr_index_list . I don't have solid details about this one.

I can't find it either. Sadness :(

andreeaflorescu

As we discussed offline I believe from_entries should not be deleted from CpuId.
I am not sure if this is useful for kvm_msr_list, but for kvm_cpuid2 we already know some scenarios where we can use it. Considering this I would suggest to add a CpuIdExtension (or similar) trait implemented for the type CpuId

andreeaflorescu · 2019-04-17T13:15:42Z

src/ioctls/mod.rs

-#[cfg(test)]
+/// Wrapper for `kvm_cpuid2`.
+///
+/// See the documentation for [KvmVec](struct.KvmVec.html)


Can you be a bit more explicit here? For example:

/// Wrapper over the `kvm_cpuid2` structure. /// /// The `kvm_cpuid2` structure has a zero sized array. For details check the /// [KVM API](https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt) /// documentation on `kvm_cpuid2`. To provide safe access to /// `kvm_cpuid_entry` elements, this type is implemented using /// [KvmVec](struct.KvmVec.html).

Thanks ! Done.

andreeaflorescu · 2019-04-17T13:17:20Z

src/ioctls/mod.rs

    }
 }

+/// Wrapper for `kvm_msr_list`.
+///
+/// See the documentation for [KvmVec](struct.KvmVec.html)


Also elaborate on KvmMsrList here and add examples.

andreeaflorescu · 2019-04-17T13:58:06Z

src/lib.rs

@@ -42,3 +43,6 @@ pub use ioctls::{KvmRunWrapper, Result};
 /// It can be used for calls to [get_supported_cpuid](struct.Kvm.html#method.get_supported_cpuid) and
 /// [get_emulated_cpuid](struct.Kvm.html#method.get_emulated_cpuid).
 pub const MAX_KVM_CPUID_ENTRIES: usize = 80;
+
+/// Maximum number of MSR entries that can be returned by a call to KVM ioctls.


I can't find it either. Sadness :(

jiangliu · 2019-04-19T08:14:45Z

Seems it's a common issue to deal with zero-sized array. Is there any best known methods for rust to support such a case? Or could we extend a basic rust-vmm crate to export helper macros/traits to deal with zero-sized array?

serban300 · 2019-04-19T11:45:13Z

Seems it's a common issue to deal with zero-sized array. Is there any best known methods for rust to support such a case? Or could we extend a basic rust-vmm crate to export helper macros/traits to deal with zero-sized array?

If zero-sized arrays are needed outside of the kvm-ioctls crate then implementing them in a separate crate with helpers / commons sounds good to me. Anyway since I'm not very familiar with rust-vmm I would need to get more opinions on this issue.

andreeaflorescu · 2019-04-19T12:09:35Z

We can ask people in the rust-vmm list what they think about this. @serban300 can you please send an email?

I believe the plan right now it to move the CpuId definition to vmm-vcpu. See rust-vmm/vmm-vcpu#3

KvmVec provides a common abstraction for all the KVM structures that ressemble arrays. Signed-off-by: Serban Iorga <seriorga@amazon.com>

Signed-off-by: Serban Iorga <seriorga@amazon.com>

sboeuf · 2019-04-23T23:41:33Z

@serban300

If zero-sized arrays are needed outside of the kvm-ioctls crate then implementing them in a separate crate with helpers / commons sounds good to me. Anyway since I'm not very familiar with rust-vmm I would need to get more opinions on this issue.

Having a common crate for this sounds like a sane idea. You should look at the existing crates already present on crates.io, maybe you will find something there.

What would be the name of the crate btw? rust-common?

I believe the plan right now it to move the CpuId definition to vmm-vcpu. See rust-vmm/vmm-vcpu#3

That's my understanding too!

andreeaflorescu · 2019-04-24T00:37:29Z

I am thinking that maybe we should instead place it in vmm-sys-utils instead of having a separate crate for it. I don't have a lot of experience here, but is this really a general problem? Do we think that in the future we will have to handle other 0-sized arrays besides the ones we have in KVM? I would be interested to see some other usecases for which this would need to live in a separate crate.

jennymankin · 2019-04-24T04:14:41Z

I think it makes sense to move the traits of kvm_vec to a common crate (or to vmm-sys-utils if that's the better option). Then the implementations of the kvm_vec for CPUID and MSR vectors would ultimately go in the Vcpu crate.

As things will no longer reside in a specific kvm-ioctls crate, and will instead be intended to be used in VMM-agnostic crates, could you rename things to avoid KVM-specific naming? File names like kvm_vec.rs, trait names like KvmVec, etc.

jennymankin · 2019-04-24T04:16:06Z

On a similar note, something I found while moving the old CpuId implementation into the Vcpu crate is that doc tests with KVM-specific or Unix-specific crates or APIs will cause cargo test to fail on Windows. In cases where the example is instructive and can't be modified to avoid such dependencies, cfg(unix) will at least prevent the doc tests from failing on Windows. For example:

    /// ```cfg(unix)
    /// extern crate kvm_ioctls;
    /// 
    /// use kvm_ioctls::Kvm;
    /// use vmm_vcpu::x86_64::{CpuId, MAX_CPUID_ENTRIES};
    /// 
    /// # fn main() {
    ///     let kvm = Kvm::new().unwrap();
    ///     let mut cpuid = kvm.get_supported_cpuid(MAX_CPUID_ENTRIES).unwrap();
    ///     let cpuid_entries = cpuid.mut_entries_slice();
    /// # }
    /// 
    /// ```

jiangliu · 2019-04-24T05:47:13Z

It’s a common case for Linux ioctl related data structures, and Vhost has similar cases. I think it’s ok to include it into the vmm-sys-utils crate.

…

On Apr 24, 2019, at 8:37 AM, Andreea Florescu ***@***.*** ***@***.***>> wrote: I am thinking that maybe we should instead place it in vmm-sys-utils instead of having a separate crate for it. I don't have a lot of experience here, but is this really a general problem? Do we think that in the future we will have to handle other 0-sized arrays besides the ones we have in KVM? I would be interested to see some other usecases for which this would need to live in a separate crate. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOXR7GQZLBIDNT2MMSGR4DPR6TUVANCNFSM4HCKZ7EA>.

serban300 · 2019-04-24T09:33:16Z

Thanks everyone for the feedback !

It would be nice if there was something similar on crates.io . I couldn't find anything so far. I will keep looking, but I think the chances are very slim.

Anyway, it seems that there is a consensus around using the vmm-sys-util crate. It looks good to me also. I will submit a PR there as soon as rust-vmm/vmm-sys-util#1 is merged. Also I will make some changes in order to avoid KVM specific naming.

alexandruag · 2019-05-20T12:22:49Z

src/ioctls/common/kvm_vec.rs

+    ///
+    fn len(&self) -> usize;
+
+    /// Get the array length as mut


This comment looks wrong.

alexandruag · 2019-05-20T12:24:39Z

src/ioctls/common/kvm_vec.rs

+    /// Get the capacity required by mem_allocator in order to hold
+    /// the provided number of  `KvmArray::Entry`
+    ///
+    fn kvm_vec_len_to_mem_allocator_len(kvm_vec_len: usize) -> usize {


I guess this could be renamed to mem_allocator_len, since the name of the parameter kinda suggests what the conversion is about.

alexandruag · 2019-05-20T12:27:22Z

src/ioctls/common/kvm_vec.rs

+        let required_mem_allocator_capacity =
+            KvmVec::<T>::kvm_vec_len_to_mem_allocator_len(num_elements);
+
+        let mut mem_allocator = Vec::with_capacity(required_mem_allocator_capacity);


I think lines 138-141 can be replaced with mem_allocator = vec![T::default(); required_mem_allocator_capacity], if it looks any better.

Yes, looks better. Done.

alexandruag · 2019-05-20T12:27:40Z

src/ioctls/common/kvm_vec.rs

+        array_size_in_bytes / size_of::<T::Entry>()
+    }
+
+    /// Constructs a new KvmVec<T> that contains `num_elements` empty elements


It might be a bit more accurate to say "default-initialized" instead of "empty" here.

alexandruag · 2019-05-20T12:29:03Z

src/ioctls/common/kvm_vec.rs

+
+        {
+            let kvm_vec_entries = kvm_vec.as_mut_kvm_struct().entries_mut();
+            // this is safe because the provided length is correct


This is for all the OCD ppl: it's nice if all the sentence-like (or longer comments) use proper capitalization and punctuation. I think there might be a couple of other places where that's not the case.

Will try to fix all the comments.

alexandruag · 2019-05-20T12:37:26Z

src/ioctls/common/kvm_vec.rs

+
+        self.reserve(1);
+
+        let mut entries = self.as_vec();


Is the self.as_vec/push combination required here? Since we've already ensured there is enough space via reserve, it looks like we can call self.update_len(desired_len);, and then update the value of the last position based on entry using the mut slice returned by self.as_mut_entries_slice.

Nice. Seems to work.

alexandruag · 2019-05-20T12:45:15Z

src/ioctls/common/kvm_vec.rs

+    /// * `f` - The function used to evaluate whether an entry will be kept or not.
+    ///         When `f` returns `true` the entry is kept.
+    ///
+    pub fn retain<P>(&mut self, f: P)


If retain remains the sole user of as_vec, which is pretty weird/scary as currently implemented, I would consider reimplementing this without as_vec (for example, using slice::sort_by, and dropping the tail; or writing the iterations + swap ourselves if that's deemed too inefficient), and removing as_vec altogether, to get rid of the extra unsafe-ness and prevent future use.

Good point. Done. I implemented some custom logic for this since it's quite easy.

alexandruag · 2019-05-20T12:46:07Z

src/ioctls/common/kvm_vec.rs

+
+impl<T: Default + KvmArray> PartialEq for KvmVec<T> {
+    fn eq(&self, other: &KvmVec<T>) -> bool {
+        self.len == other.len && self.as_entries_slice() == other.as_entries_slice()


Is self.len == other.len implied by self.as_entries_slice() == other.as_entries_slice()?

Not necessarily. If something goes wrong self.len can differ from T.len()

alexandruag · 2019-05-20T12:46:26Z

src/ioctls/common/kvm_vec.rs

+
+        let num_bytes = self.mem_allocator.len() * size_of::<T>();
+        let src_byte_slice =
+            unsafe { std::slice::from_raw_parts(self.as_ptr() as *const u8, num_bytes) };


Courtesy comment regarding why the unsafe block is safe is nice.

I changed the implementation in order to avoid using unsafe:

clone.mem_allocator = self.mem_allocator.to_vec();

alexandruag · 2019-05-20T12:46:32Z

src/ioctls/common/kvm_vec.rs

+        let src_byte_slice =
+            unsafe { std::slice::from_raw_parts(self.as_ptr() as *const u8, num_bytes) };
+        let dst_byte_slice =
+            unsafe { std::slice::from_raw_parts_mut(clone.as_mut_ptr() as *mut u8, num_bytes) };


The same comment as above.

serban300 · 2019-05-31T13:24:55Z

Closing in favor of rust-vmm/vmm-sys-util#4

serban300 mentioned this pull request Mar 29, 2019

Scope use_host_cpuid_function refactoring firecracker-microvm/firecracker#1019

Closed

acatangiu reviewed Mar 29, 2019

View reviewed changes

acatangiu reviewed Apr 1, 2019

View reviewed changes

acatangiu previously approved these changes Apr 2, 2019

View reviewed changes

acatangiu reviewed Apr 5, 2019

View reviewed changes

src/ioctls/kvm_vec.rs Outdated Show resolved Hide resolved

acatangiu reviewed Apr 5, 2019

View reviewed changes

src/ioctls/kvm_vec.rs Outdated Show resolved Hide resolved

andreeaflorescu reviewed Apr 5, 2019

View reviewed changes

src/ioctls/kvm_vec.rs Outdated Show resolved Hide resolved

src/ioctls/system.rs Show resolved Hide resolved

serban300 dismissed acatangiu’s stale review via fbf7672 April 9, 2019 10:26

serban300 self-assigned this Apr 9, 2019

andreeaflorescu reviewed Apr 12, 2019

View reviewed changes

acatangiu previously approved these changes Apr 16, 2019

View reviewed changes

serban300 dismissed acatangiu’s stale review via 0ed9715 April 17, 2019 08:59

andreeaflorescu reviewed Apr 17, 2019

View reviewed changes

andreeaflorescu mentioned this pull request Apr 19, 2019

Implementation of vCPU crate rust-vmm/vmm-vcpu#3

Open

Serban Iorga added 2 commits April 23, 2019 14:57

implement KvmVec struct

41657d5

KvmVec provides a common abstraction for all the KVM structures that ressemble arrays. Signed-off-by: Serban Iorga <seriorga@amazon.com>

implement CpuId and MsrList using KvmVec

bb9e5c7

Signed-off-by: Serban Iorga <seriorga@amazon.com>

alexandruag reviewed May 20, 2019

View reviewed changes

serban300 closed this May 31, 2019

	let entries_ptr = self.as_mut_original_struct().entries_mut().as_mut_ptr();
	let entries_ptr = self.kvm_array[0].entries_mut().as_mut_ptr();

implement KvmVec struct #6

implement KvmVec struct #6

Conversation

serban300 commented Mar 29, 2019

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

andreeaflorescu left a comment

Choose a reason for hiding this comment

serban300 commented Apr 9, 2019 • edited Loading

andreeaflorescu commented Apr 9, 2019

serban300 commented Apr 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Apr 19, 2019

serban300 commented Apr 19, 2019

andreeaflorescu commented Apr 19, 2019

sboeuf commented Apr 23, 2019

andreeaflorescu commented Apr 24, 2019

jennymankin commented Apr 24, 2019

jennymankin commented Apr 24, 2019 • edited Loading

jiangliu commented Apr 24, 2019 via email

serban300 commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 commented May 31, 2019

serban300 commented Apr 9, 2019 •

edited

Loading

serban300 Apr 12, 2019 •

edited

Loading

jennymankin commented Apr 24, 2019 •

edited

Loading