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.

Sign up for GitHub

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

Jump to bottom

Clean up NVMe PRP parsing and add tests #468

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Clean up NVMe PRP parsing and add tests #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/propolis/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,11 @@ impl RWOp<'_, '_> {
}

/// An address within a guest VM.
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub struct GuestAddr(pub u64);

/// A region of memory within a guest VM.
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub struct GuestRegion(pub GuestAddr, pub usize);

impl Add<usize> for GuestAddr {
Expand Down
247 changes: 221 additions & 26 deletions lib/propolis/src/hw/nvme/cmds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,12 +602,12 @@ impl PrpIter<'_> {
assert!(self.error.is_none());

// PRP Entry Layout
// | 63 . . . . . . . . . . . . . . . . . . . n + 1 | n . . . . . . 2 | 1 0 |
// | page base address | offset | 0 0 |
// | 63 . . . . . . . . . . . . . . . n + 1 | n . . . . . . 2 | 1 0 |
// | page base address | offset | 0 0 |
let (addr, size, next) = match self.next {
PrpNext::Prp1 => {
// The first PRP entry contained within the command may have a non-zero offset
// within the memory page.
// The first PRP entry contained within the command may have a
// non-zero offset within the memory page.
let offset = self.prp1 & PAGE_OFFSET as u64;
let size = u64::min(PAGE_SIZE as u64 - offset, self.remain);
let after = self.remain - size;
Expand All @@ -618,7 +618,8 @@ impl PrpIter<'_> {
// entry which should be present in PRP2
PrpNext::Prp2
} else {
// If the remaining length is larger than the page size, PRP2 points to a list.
// If the remaining length is larger than the page size,
// PRP2 points to a list.
//
// The first PRP List entry:
// - shall be Qword aligned, and
Expand All @@ -627,9 +628,10 @@ impl PrpIter<'_> {
return Err("PRP2 not Qword aligned!");
}

// PRP2 is allowed a non-zero offset into the page, meaning this operation's
// PRP list could start in the middle of the page. For example, idx below could
// be anywhere from 0 to PRP_LIST_MAX:
// PRP2 is allowed a non-zero offset into the page, meaning
// this operation's PRP list could start in the middle of
// the page. For example, idx below could be anywhere from 0
// to PRP_LIST_MAX:
//
// | idx
// --------------| ---
Expand All @@ -640,9 +642,10 @@ impl PrpIter<'_> {
// PRP List base | PRP_LIST_MAX - 1
// PRP List base | PRP_LIST_MAX
//
// Note that lists cannot cross page boundaries. If idx = PRP_LIST_MAX is
// reached, the last entry will point to another list (unless the remaining
// size is satisfied by the end of the list).
// Note that lists cannot cross page boundaries. If idx =
// PRP_LIST_MAX is reached, the last entry will point to
// another list (unless the remaining size is satisfied by
// the end of the list).
let base = self.prp2 & (PAGE_MASK as u64);
let idx = (self.prp2 & PAGE_OFFSET as u64) / 8;
PrpNext::List(base, idx as u16)
Expand Down Expand Up @@ -673,25 +676,18 @@ impl PrpIter<'_> {

if self.remain <= PAGE_SIZE as u64 {
(entry, self.remain, PrpNext::Done)
} else if idx != PRP_LIST_MAX {
(entry, PAGE_SIZE as u64, PrpNext::List(base, idx + 1))
} else {
if idx != PRP_LIST_MAX {
(entry, PAGE_SIZE as u64, PrpNext::List(base, idx + 1))
} else {
// Chase the PRP to the next PRP list and read the first entry from it to
// use as the next result.
let next_entry: u64 =
self.mem.read(GuestAddr(entry)).ok_or_else(
|| "Unable to read PRP list entry",
)?;
if next_entry & PAGE_OFFSET as u64 != 0 {
return Err("Inappropriate PRP list entry offset");
}
(next_entry, PAGE_SIZE as u64, PrpNext::List(entry, 1))
}
// The last PRP in this list chains to another
// (page-aligned) list with the next PRP.
self.next = PrpNext::List(entry, 0);
return self.get_next();
Comment on lines +684 to +685
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup

}
}
PrpNext::Done => {
// prior checks of self.remain should prevent us from ever reaching this
// prior checks of self.remain should prevent us from ever
// reaching this
panic!()
}
};
Expand Down Expand Up @@ -830,3 +826,202 @@ impl From<block::Result> for Completion {
}
}
}

#[cfg(test)]
mod test {
use std::sync::Arc;

use crate::common::*;
use crate::vmm::mem::{MemCtx, PhysMap};

use super::PrpIter;

const VM_SIZE: usize = 256 * PAGE_SIZE;
const PRP_PER_PAGE: usize = PAGE_SIZE / 8;

fn setup() -> (PhysMap, Arc<MemCtx>) {
let mut pmap = PhysMap::new_test(VM_SIZE);
pmap.add_test_mem("lowmem".to_string(), 0, VM_SIZE)
.expect("lowmem seg creation should succeed");

let memctx = pmap.memctx();
(pmap, memctx)
}

// Simple helpers to make math below more terse
const fn region(addr: u64, sz: u64) -> GuestRegion {
GuestRegion(GuestAddr(addr), sz as usize)
}
const fn pages(c: u64) -> u64 {
PAGE_SIZE as u64 * c
}

#[test]
fn test_prp_single() {
let (_pmap, memctx) = setup();

// Basic single page
let mut iter = PrpIter::new(pages(1), 0x1000, 0, &memctx);
assert_eq!(iter.next(), Some(region(0x1000, pages(1))));
assert_eq!(iter.next(), None);

// Sub-single page
let sub = 0x200;
let mut iter = PrpIter::new(pages(1) - sub, 0x1000, 0, &memctx);
assert_eq!(iter.next(), Some(region(0x1000, pages(1) - sub)));
assert_eq!(iter.next(), None);

// Sub-single page (with offset)
let mut iter = PrpIter::new(pages(1) - sub, 0x1000 + sub, 0, &memctx);
assert_eq!(iter.next(), Some(region(0x1000 + sub, pages(1) - sub)));
assert_eq!(iter.next(), None);
}

#[test]
fn test_prp_dual() {
let (_pmap, memctx) = setup();

// Basic dual page
let mut iter = PrpIter::new(pages(2), 0x1000, 0x2000, &memctx);
assert_eq!(iter.next(), Some(region(0x1000, pages(1))));
assert_eq!(iter.next(), Some(region(0x2000, pages(1))));
assert_eq!(iter.next(), None);

// single page, split by offset
let sub = 0x200;
let mut iter = PrpIter::new(pages(1), 0x1000 + sub, 0x2000, &memctx);
assert_eq!(iter.next(), Some(region(0x1000 + sub, pages(1) - sub)));
assert_eq!(iter.next(), Some(region(0x2000, sub)));
assert_eq!(iter.next(), None);

// less than single page, split by offset
let sz = pages(1) - 0x200;
let off = 0x400;
let mut iter = PrpIter::new(sz, 0x1000 + off, 0x2000, &memctx);
assert_eq!(iter.next(), Some(region(0x1000 + off, pages(1) - off)));
assert_eq!(iter.next(), Some(region(0x2000, sz - (pages(1) - off))));
assert_eq!(iter.next(), None);
}

#[test]
fn test_prp_list() {
// Basic triple page (aligned prplist)
let (_pmap, memctx) = setup();
let listprps: [u64; 2] = [0x2000, 0x3000];
let listaddr = 0x80000;
memctx.write(GuestAddr(listaddr), &listprps);
let mut iter = PrpIter::new(pages(3), 0x1000, listaddr, &memctx);
assert_eq!(iter.next(), Some(region(0x1000, pages(1))));
assert_eq!(iter.next(), Some(region(0x2000, pages(1))));
assert_eq!(iter.next(), Some(region(0x3000, pages(1))));
assert_eq!(iter.next(), None);

// Basic triple page (offset prplist)
let (_pmap, memctx) = setup();
let listprps: [u64; 2] = [0x2000, 0x3000];
let listaddr = 0x80010;
memctx.write(GuestAddr(listaddr), &listprps);
let mut iter = PrpIter::new(pages(3), 0x1000, listaddr, &memctx);
assert_eq!(iter.next(), Some(region(0x1000, pages(1))));
assert_eq!(iter.next(), Some(region(0x2000, pages(1))));
assert_eq!(iter.next(), Some(region(0x3000, pages(1))));
assert_eq!(iter.next(), None);

// Offset triple page
let (_pmap, memctx) = setup();
let listprps: [u64; 3] = [0x2000, 0x3000, 0x4000];
let listaddr = 0x80000;
let off = 0x200;
memctx.write(GuestAddr(listaddr), &listprps);
let mut iter = PrpIter::new(pages(3), 0x1000 + off, listaddr, &memctx);
assert_eq!(iter.next(), Some(region(0x1000 + off, pages(1) - off)));
assert_eq!(iter.next(), Some(region(0x2000, pages(1))));
assert_eq!(iter.next(), Some(region(0x3000, pages(1))));
assert_eq!(iter.next(), Some(region(0x4000, off)));
assert_eq!(iter.next(), None);
}

#[test]
fn test_prp_list_offset_last() {
// List with offset, where last entry covers less than one page
let (_pmap, memctx) = setup();
let listaddr = 0x80000u64;
let mut prps: Vec<u64> = Vec::with_capacity(PRP_PER_PAGE);
let mut bufaddr = 0x2000u64;
for _idx in 0..PRP_PER_PAGE {
prps.push(bufaddr);
bufaddr += pages(1);
}
memctx.write_many(GuestAddr(listaddr), &prps);
let off = 0x200;
let mut iter = PrpIter::new(
pages(PRP_PER_PAGE as u64),
0x1000 + off,
listaddr,
&memctx,
);
assert_eq!(
iter.next(),
Some(region(0x1000 + off, pages(1) - off)),
"prp1 entry incorrect"
);

let mut bufaddr = 0x2000u64;
for idx in 0..(PRP_PER_PAGE - 1) {
assert_eq!(
iter.next(),
Some(region(bufaddr, pages(1))),
"bad prp at idx: {idx}"
);
bufaddr += pages(1);
}
// last prplist entry should be the remaining bytes left over from
// offsetting the prp1 entry
assert_eq!(iter.next(), Some(region(bufaddr, off)));
}

#[test]
fn test_prp_multiple() {
// Basic multiple-page prplist
let (_pmap, memctx) = setup();
let listaddrs = [0x80000u64, 0x81000u64];
let mut prps: Vec<u64> = Vec::with_capacity(PRP_PER_PAGE);
let mut bufaddr = 0x2000u64;

let entries_first = PRP_PER_PAGE - 1;
for _idx in 0..entries_first {
prps.push(bufaddr);
bufaddr += pages(1);
}
// Link to the next list page
prps.push(listaddrs[1]);
memctx.write_many(GuestAddr(listaddrs[0]), &prps[..]);

// populate a few more entries in the next prplist
let entries_second = 4;
for idx in 0..entries_second {
prps[idx] = bufaddr;
bufaddr += pages(1);
}
memctx.write_many(GuestAddr(listaddrs[1]), &prps[..entries_second]);

let total_entries = 1 + entries_first + entries_second;
let mut iter = PrpIter::new(
pages(total_entries as u64),
0x1000,
listaddrs[0],
&memctx,
);

let mut bufaddr = 0x1000u64;
for idx in 0..total_entries {
assert_eq!(
iter.next(),
Some(region(bufaddr, pages(1))),
"bad prp at idx: {idx}"
);
bufaddr += pages(1);
}
assert_eq!(iter.next(), None);
}
}
Loading
Loading