From e7c46cad4150ecb18a0832a61042522974543938 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Mon, 21 Oct 2019 17:02:40 -0700 Subject: devices: virtio: block: refactor status_writer setup This consolidates the status byte manipulation in process_one_request() instead of requiring both that function and execute_request() to deal with it. The tests are modified to run the full process_one_request() function instead of just execute_request() to exercise the full descriptor parsing logic, and they are adapted to read the status of the request from the status byte in the buffer from the descriptor since process_one_request() returns successfully as long as the descriptor parsing succeeded, even if the requested I/O failed. BUG=None TEST=./build_test Change-Id: I17affabc2d3c30c810643ce260152cf34893b772 Signed-off-by: Daniel Verkamp Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1918479 Reviewed-by: Dylan Reid Reviewed-by: Chirantan Ekbote Tested-by: kokoro --- devices/src/virtio/block.rs | 48 ++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 20 deletions(-) (limited to 'devices/src/virtio/block.rs') diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs index 5c01316..e3e5247 100644 --- a/devices/src/virtio/block.rs +++ b/devices/src/virtio/block.rs @@ -261,25 +261,28 @@ impl Worker { flush_timer_armed: &mut bool, mem: &GuestMemory, ) -> result::Result { - let mut status_writer = - Writer::new(mem, avail_desc.clone()).map_err(ExecuteError::Descriptor)?; - let available_bytes = status_writer.available_bytes(); + let mut reader = Reader::new(mem, avail_desc.clone()).map_err(ExecuteError::Descriptor)?; + let mut writer = Writer::new(mem, avail_desc).map_err(ExecuteError::Descriptor)?; + + // The last byte of the buffer is virtio_blk_req::status. + // Split it into a separate Writer so that status_writer is the final byte and + // the original writer is left with just the actual block I/O data. + let available_bytes = writer.available_bytes(); let status_offset = available_bytes .checked_sub(1) .ok_or(ExecuteError::MissingStatus)?; - - status_writer = status_writer + let mut status_writer = writer .split_at(status_offset) .map_err(ExecuteError::Descriptor)?; let status = match Block::execute_request( - avail_desc, + &mut reader, + &mut writer, read_only, disk, disk_size, flush_timer, flush_timer_armed, - mem, ) { Ok(()) => VIRTIO_BLK_S_OK, Err(e) => { @@ -530,18 +533,19 @@ impl Block { }) } + // Execute a single block device request. + // `writer` includes the data region only; the status byte is not included. + // It is up to the caller to convert the result of this function into a status byte + // and write it to the expected location in guest memory. fn execute_request( - avail_desc: DescriptorChain, + reader: &mut Reader, + writer: &mut Writer, read_only: bool, disk: &mut dyn DiskFile, disk_size: u64, flush_timer: &mut TimerFd, flush_timer_armed: &mut bool, - mem: &GuestMemory, ) -> result::Result<(), ExecuteError> { - let mut reader = Reader::new(mem, avail_desc.clone()).map_err(ExecuteError::Descriptor)?; - let mut writer = Writer::new(mem, avail_desc).map_err(ExecuteError::Descriptor)?; - let req_header: virtio_blk_req_header = reader.read_obj().map_err(ExecuteError::Read)?; let req_type = req_header.req_type.to_native(); @@ -574,11 +578,7 @@ impl Block { match req_type { VIRTIO_BLK_T_IN => { - // The last byte of writer is virtio_blk_req::status, so subtract it from data_len. - let data_len = writer - .available_bytes() - .checked_sub(1) - .ok_or(ExecuteError::MissingStatus)?; + let data_len = writer.available_bytes(); let offset = sector .checked_shl(u32::from(SECTOR_SHIFT)) .ok_or(ExecuteError::OutOfRange)?; @@ -881,7 +881,7 @@ mod tests { let mut flush_timer = TimerFd::new().expect("failed to create flush_timer"); let mut flush_timer_armed = false; - Block::execute_request( + Worker::process_one_request( avail_desc, false, &mut f, @@ -891,6 +891,10 @@ mod tests { &mem, ) .expect("execute failed"); + + let status_offset = GuestAddress((0x1000 + size_of_val(&req_hdr) + 512) as u64); + let status = mem.read_obj_from_addr::(status_offset).unwrap(); + assert_eq!(status, VIRTIO_BLK_S_OK); } #[test] @@ -937,7 +941,7 @@ mod tests { let mut flush_timer = TimerFd::new().expect("failed to create flush_timer"); let mut flush_timer_armed = false; - Block::execute_request( + Worker::process_one_request( avail_desc, false, &mut f, @@ -946,6 +950,10 @@ mod tests { &mut flush_timer_armed, &mem, ) - .expect_err("execute was supposed to fail"); + .expect("execute failed"); + + let status_offset = GuestAddress((0x1000 + size_of_val(&req_hdr) + 512 * 2) as u64); + let status = mem.read_obj_from_addr::(status_offset).unwrap(); + assert_eq!(status, VIRTIO_BLK_S_IOERR); } } -- cgit 1.4.1