Skip to content

Commit 70bce30

Browse files
committed
refactor: replace read_obj in the descriptor chain creation
`read_obj` takes too much time and we don't need it's checks as we do them before the actual reading happens. To help with perpormance, replace it with either `get_slice` or `get_host_address`. Signed-off-by: Egor Lazarchuk <[email protected]>
1 parent b64a2d4 commit 70bce30

File tree

1 file changed

+26
-15
lines changed

1 file changed

+26
-15
lines changed

src/vmm/src/devices/virtio/queue.rs

+26-15
Original file line numberDiff line numberDiff line change
@@ -113,17 +113,16 @@ impl<'a, M: GuestMemory> DescriptorChain<'a, M> {
113113
// bounds.
114114
let desc_head = desc_table.unchecked_add(u64::from(index) * 16);
115115

116-
// These reads can't fail unless Guest memory is hopelessly broken.
117-
let desc = match mem.read_obj::<Descriptor>(desc_head) {
118-
Ok(ret) => ret,
119-
Err(err) => {
120-
error!(
121-
"Failed to read virtio descriptor from memory at address {:#x}: {}",
122-
desc_head.0, err
123-
);
124-
return None;
125-
}
126-
};
116+
// SAFETY:
117+
// This can't fail as we checked the `desc_head`
118+
let ptr = mem.get_host_address(desc_head).unwrap();
119+
120+
// SAFETY:
121+
// Safe as we know that `ptr` is inside guest memory and
122+
// following `std::mem::size_of::<Descriptor>` bytes belong
123+
// to the descriptor table
124+
let desc: &Descriptor = unsafe { &*ptr.cast::<Descriptor>() };
125+
127126
let chain = DescriptorChain {
128127
mem,
129128
desc_table,
@@ -423,7 +422,12 @@ impl Queue {
423422
// `self.is_valid()` already performed all the bound checks on the descriptor table
424423
// and virtq rings, so it's safe to unwrap guest memory reads and to use unchecked
425424
// offsets.
426-
let desc_index: u16 = mem.read_obj(desc_index_address).unwrap();
425+
let slice = mem
426+
.get_slice(desc_index_address, std::mem::size_of::<u16>())
427+
.unwrap();
428+
// SAFETY:
429+
// We transforming valid memory slice
430+
let desc_index = unsafe { *slice.ptr_guard().as_ptr().cast::<u16>() };
427431

428432
DescriptorChain::checked_new(mem, self.desc_table, self.actual_size(), desc_index).map(
429433
|dc| {
@@ -1221,9 +1225,6 @@ mod tests {
12211225
// index >= queue_size
12221226
assert!(DescriptorChain::checked_new(m, vq.dtable_start(), 16, 16).is_none());
12231227

1224-
// desc_table address is way off
1225-
assert!(DescriptorChain::checked_new(m, GuestAddress(0x00ff_ffff_ffff), 16, 0).is_none());
1226-
12271228
// Let's create an invalid chain.
12281229
{
12291230
// The first desc has a normal len, and the next_descriptor flag is set.
@@ -1257,6 +1258,16 @@ mod tests {
12571258
}
12581259
}
12591260

1261+
#[test]
1262+
#[should_panic]
1263+
fn test_checked_new_descriptor_chain_panic() {
1264+
let m = &multi_region_mem(&[(GuestAddress(0), 0x10000)]);
1265+
1266+
// `checked_new` does assume that `desc_table` is valid.
1267+
// When desc_table address is way off, it should panic.
1268+
DescriptorChain::checked_new(m, GuestAddress(0x00ff_ffff_ffff), 16, 0);
1269+
}
1270+
12601271
#[test]
12611272
fn test_queue_validation() {
12621273
let m = &default_mem();

0 commit comments

Comments
 (0)