pvmfw: virtio: Clean up hal.rs
Replace contains_range() with the RangeExt::is_within helper.
Turn the logging calls tracking VirtIO buffer management into trace!().
Move logging for allocation of shared memory to the allocators.
Avoid using as when casting pointers and use methods instead; in
particular, explicitly const_cast the source of a copy_nonoverlapping.
Make the logs for copying to/from bounce buffers easier to grep/parse.
Minimize scope of unsafe blocks, add missing SAFETY comments, and
clarify that HalImpl methods safety requirements are documented in the
trait.
Add copyright header and module docstring.
Bug: 280644106
Test: atest MicrodroidTests
Change-Id: I1b4daade70f5f43b6bc0f222568fbaaa29edf27f
diff --git a/pvmfw/src/memory.rs b/pvmfw/src/memory.rs
index a2b7e09..16c1a37 100644
--- a/pvmfw/src/memory.rs
+++ b/pvmfw/src/memory.rs
@@ -34,6 +34,7 @@
use core::result;
use hyp::get_hypervisor;
use log::error;
+use log::trace;
use once_cell::race::OnceBox;
use spin::mutex::SpinMutex;
use tinyvec::ArrayVec;
@@ -317,6 +318,7 @@
/// is not aligned with the memory protection granule then it will be extended on either end to
/// align.
fn share_range(range: &MemoryRange, granule: usize) -> hyp::Result<()> {
+ trace!("Sharing memory region {range:#x?}");
for base in (align_down(range.start, granule)
.expect("Memory protection granule was not a power of two")..range.end)
.step_by(granule)
@@ -330,6 +332,7 @@
/// shared. If the range is not aligned with the memory protection granule then it will be extended
/// on either end to align.
fn unshare_range(range: &MemoryRange, granule: usize) -> hyp::Result<()> {
+ trace!("Unsharing memory region {range:#x?}");
for base in (align_down(range.start, granule)
.expect("Memory protection granule was not a power of two")..range.end)
.step_by(granule)
@@ -356,6 +359,7 @@
handle_alloc_error(layout);
};
+ trace!("Allocated shared buffer at {buffer:?} with {layout:?}");
return Ok(buffer);
}
@@ -372,6 +376,7 @@
// be reused while maybe still partially shared with the host.
share_range(&(paddr..paddr + layout.size()), granule)?;
+ trace!("Allocated shared memory at {buffer:?} with {layout:?}");
Ok(buffer)
}
@@ -392,6 +397,7 @@
// the same allocator, and the layout is the same as was used then.
unsafe { shared_pool.dealloc(vaddr.as_ptr(), layout) };
+ trace!("Deallocated shared buffer at {vaddr:?} with {layout:?}");
return Ok(());
}
@@ -401,6 +407,7 @@
// the layout is the same as was used then.
unsafe { dealloc(vaddr.as_ptr(), layout) };
+ trace!("Deallocated shared memory at {vaddr:?} with {layout:?}");
Ok(())
}
diff --git a/pvmfw/src/virtio/hal.rs b/pvmfw/src/virtio/hal.rs
index 7598a55..41d0f24 100644
--- a/pvmfw/src/virtio/hal.rs
+++ b/pvmfw/src/virtio/hal.rs
@@ -1,10 +1,24 @@
+// Copyright 2023, The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! HAL for the virtio_drivers crate.
+
use super::pci::PCI_INFO;
+use crate::helpers::RangeExt as _;
use crate::memory::{alloc_shared, dealloc_shared, phys_to_virt, virt_to_phys};
-use core::{
- ops::Range,
- ptr::{copy_nonoverlapping, NonNull},
-};
-use log::debug;
+use core::ptr::{copy_nonoverlapping, NonNull};
+use log::trace;
use virtio_drivers::{BufferDirection, Hal, PhysAddr, PAGE_SIZE};
pub struct HalImpl;
@@ -13,8 +27,8 @@
///
/// # Safety
///
-/// Callers of this implementatation must follow the safety requirements documented for the unsafe
-/// methods.
+/// Callers of this implementatation must follow the safety requirements documented in the `Hal`
+/// trait for the unsafe methods.
unsafe impl Hal for HalImpl {
/// Allocates the given number of contiguous physical pages of DMA memory for VirtIO use.
///
@@ -25,7 +39,6 @@
/// block of memory using `alloc_shared` and returning a non-null pointer to it that is
/// aligned to `PAGE_SIZE`.
fn dma_alloc(pages: usize, _direction: BufferDirection) -> (PhysAddr, NonNull<u8>) {
- debug!("dma_alloc: pages={}", pages);
let size = pages * PAGE_SIZE;
let vaddr =
alloc_shared(size).expect("Failed to allocate and share VirtIO DMA range with host");
@@ -33,14 +46,11 @@
(paddr, vaddr)
}
- unsafe fn dma_dealloc(paddr: PhysAddr, vaddr: NonNull<u8>, pages: usize) -> i32 {
- debug!("dma_dealloc: paddr={:#x}, pages={}", paddr, pages);
+ unsafe fn dma_dealloc(_paddr: PhysAddr, vaddr: NonNull<u8>, pages: usize) -> i32 {
let size = pages * PAGE_SIZE;
- // Safe because the memory was allocated by `dma_alloc` above using the same allocator, and
- // the layout is the same as was used then.
- unsafe {
- dealloc_shared(vaddr, size).expect("Failed to unshare VirtIO DMA range with host");
- }
+ // SAFETY - Memory was allocated by `dma_alloc` using `alloc_shared` with the same size.
+ unsafe { dealloc_shared(vaddr, size) }
+ .expect("Failed to unshare VirtIO DMA range with host");
0
}
@@ -52,19 +62,21 @@
/// is within the PCI MMIO range.
unsafe fn mmio_phys_to_virt(paddr: PhysAddr, size: usize) -> NonNull<u8> {
let pci_info = PCI_INFO.get().expect("VirtIO HAL used before PCI_INFO was initialised");
+ let bar_range = {
+ let start = pci_info.bar_range.start.try_into().unwrap();
+ let end = pci_info.bar_range.end.try_into().unwrap();
+
+ start..end
+ };
+ let mmio_range = paddr..paddr.checked_add(size).expect("PCI MMIO region end overflowed");
+
// Check that the region is within the PCI MMIO range that we read from the device tree. If
// not, the host is probably trying to do something malicious.
- if !contains_range(
- &pci_info.bar_range,
- &(paddr.try_into().expect("PCI MMIO region start was outside of 32-bit address space")
- ..paddr
- .checked_add(size)
- .expect("PCI MMIO region end overflowed")
- .try_into()
- .expect("PCI MMIO region end was outside of 32-bit address space")),
- ) {
- panic!("PCI MMIO region was outside of expected BAR range.");
- }
+ assert!(
+ mmio_range.is_within(&bar_range),
+ "PCI MMIO region was outside of expected BAR range.",
+ );
+
phys_to_virt(paddr)
}
@@ -73,41 +85,31 @@
// TODO: Copy to a pre-shared region rather than allocating and sharing each time.
// Allocate a range of pages, copy the buffer if necessary, and share the new range instead.
- let copy =
- alloc_shared(size).expect("Failed to allocate and share VirtIO buffer with host");
+ let bounce = alloc_shared(size)
+ .expect("Failed to allocate and share VirtIO bounce buffer with host");
+ let paddr = virt_to_phys(bounce);
if direction == BufferDirection::DriverToDevice {
- unsafe {
- copy_nonoverlapping(buffer.as_ptr() as *mut u8, copy.as_ptr(), size);
- }
+ let src = buffer.cast::<u8>().as_ptr().cast_const();
+ trace!("VirtIO bounce buffer at {bounce:?} (PA:{paddr:#x}) initialized from {src:?}");
+ // SAFETY - Both regions are valid, properly aligned, and don't overlap.
+ unsafe { copy_nonoverlapping(src, bounce.as_ptr(), size) };
}
- virt_to_phys(copy)
+
+ paddr
}
unsafe fn unshare(paddr: PhysAddr, buffer: NonNull<[u8]>, direction: BufferDirection) {
- let vaddr = phys_to_virt(paddr);
+ let bounce = phys_to_virt(paddr);
let size = buffer.len();
if direction == BufferDirection::DeviceToDriver {
- debug!(
- "Copying VirtIO buffer back from {:#x} to {:#x}.",
- paddr,
- buffer.as_ptr() as *mut u8 as usize
- );
- unsafe {
- copy_nonoverlapping(vaddr.as_ptr(), buffer.as_ptr() as *mut u8, size);
- }
+ let dest = buffer.cast::<u8>().as_ptr();
+ trace!("VirtIO bounce buffer at {bounce:?} (PA:{paddr:#x}) copied back to {dest:?}");
+ // SAFETY - Both regions are valid, properly aligned, and don't overlap.
+ unsafe { copy_nonoverlapping(bounce.as_ptr(), dest, size) };
}
- // Unshare and deallocate the shared copy of the buffer.
- debug!("Unsharing VirtIO buffer {:#x}", paddr);
- // Safe because the memory was allocated by `share` using `alloc_shared`, and the size is
- // the same as was used then.
- unsafe {
- dealloc_shared(vaddr, size).expect("Failed to unshare VirtIO buffer with host");
- }
+ // SAFETY - Memory was allocated by `share` using `alloc_shared` with the same size.
+ unsafe { dealloc_shared(bounce, size) }
+ .expect("Failed to unshare and deallocate VirtIO bounce buffer");
}
}
-
-/// Returns true if `inner` is entirely contained within `outer`.
-fn contains_range(outer: &Range<u32>, inner: &Range<u32>) -> bool {
- inner.start >= outer.start && inner.end <= outer.end
-}