Design: refactoring read_physical API

[Wenzel]

In light of the work we already accomplished with https://github.com/Wenzel/libmicrovmi/pull/165, I would like to open this design issue to provide some ideas about refactoring the read_physical API, and implementation.

read_physical_padded

It would be convient to have a read_physical_padded() API,directly available in the Introspectable trait, instead of implementing in the Python layer, at PaddedPhysicalMemoryIO. It would be more efficient, and available to the C and Rust programs.

moving read algorithms from driver to Introspectable trait

If we take a look at the current implementation for Xen and KVM

• xen read_physical
• kvm read_physical

Both of them will split the read by 4K chunks, the size of a page.

the proposal would be to move a maximum of this common read algorithm into the Introspectable trait

Proposal

use std::io::Read;

trait Introspectable {
  fn read_physical(paddr: u64, buf: &mut [u8], bytes_read: &mut u64) -> Result<(), Box<dyn Error>> {
     // implementation provided in the trait
    for (i, chunk) in buf.chunks_mut(PAGE_SIZE).enumerate() {
      let new_gfn = xxxx;
      let page = self.get_physical_page(gfn)?;
      page.read(PAGE_SIZE, chunk)?;
    }
  }

  fn read_physical_padded(paddr: u64, buf: &mut [u8], bytes_read: &mut u64) -> Result<(), Box<dyn Error>> {
    // same as above, but doesn't stop when a page is missing, fill with zeroes instead
  }

  // Return a Readable object that represents a physical page (or frame)
  fn get_physical_page(gfn: u64) -> Result<Box<dyn Read>, Box<dyn Error>>;
}

With this solution we factorise read operation code in the trait, and we can handle the Xen situation where a page has to be mapped / unmapped by returning a Boxed object, that can implement a Drop trait and handle unmapping on deallocation.

[Wenzel]

I'm also wondering how we could push the Read trait further into the user API ? Afterall, we should reuse a maximum of rust traits, and avoiding inventing unecessary APIs. Also, read_physical() can stay for convenience, but it's implementation could rely on the Read + Seek traits underneath.

Ideas ?

let mut drv = microvmi::init(domain_name, None, init_option).unwrap();

// Introspectable trait also implements Read trait
drv.read()

// or
// return a 'memory' object, which implements the read trait
let memory = drv.memory()
memory.read()
// and we can return a second memory object, which implements a padded read
let padded_mem = drv.padded_memory()
padded_mem.read()

[Wenzel]

Design in progress here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=129baa6895e5f0a1e65415eb9b94fd02

One thing I learned: we don't need 2 methods memory() and padded_memory().

The Read trait has a read_exact() function that can be reimplemented, and used as a read_physical_padded().