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.

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

RFE: Skeleton for DMA layer #306

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
89 changes: 89 additions & 0 deletions openhcl/underhill_core/src/dma_client.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
use std::sync::{Arc, Weak};
use crate::dma_manager::GlobalDmaManager;
use user_driver::dma;

// DMA Client structure, representing a specific client instance
pub struct DmaClient {
manager: Weak<GlobalDmaManager>,
}

impl DmaClient {
pub fn new(manager: Weak<GlobalDmaManager>) -> Self {
Self { manager }
}

fn pin_memory(&self, range: &MemoryRange) -> Result<usize, DmaError> {
let manager = self.manager.upgrade().ok_or(DmaError::InitializationFailed)?;
let threshold = manager.get_client_threshold(self).ok_or(DmaError::InitializationFailed)?;

if range.size <= threshold && manager.is_pinned(range) {
Ok(range.start)
} else {
Err(DmaError::PinFailed)
}
}

pub fn map_dma_ranges(
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is a draft ... could you add a doc comment so that folks know the intended contract and usage here?

&self,
ranges: &[MemoryRange],
) -> Result<DmaTransactionHandler, DmaError> {
let manager = self.manager.upgrade().ok_or(DmaError::InitializationFailed)?;
let mut dma_transactions = Vec::new();

let threshold = manager.get_client_threshold(self).ok_or(DmaError::InitializationFailed)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be moved before defining dma_transactions to fail earlier.


for range in ranges {
let (dma_addr, is_pinned, is_bounce_buffer) = if range.size <= threshold {
match self.pin_memory(range) {
Ok(pinned_addr) => (pinned_addr, true, false),
Err(_) => {
let bounce_addr = manager.allocate_bounce_buffer(range.size)?;
(bounce_addr, false, true)
}
}
} else {
let bounce_addr = manager.allocate_bounce_buffer(range.size)?;
(bounce_addr, false, true)
};

dma_transactions.push(DmaTransaction {
original_addr: range.start,
dma_addr,
size: range.size,
is_pinned,
is_bounce_buffer,
is_physical: !is_bounce_buffer,
is_prepinned: manager.is_pinned(range),
});
}

Ok(DmaTransactionHandler {
transactions: dma_transactions,
})
}

pub fn unmap_dma_ranges(&self, dma_transactions: &[DmaTransaction]) -> Result<(), DmaError> {
let manager = self.manager.upgrade().ok_or(DmaError::InitializationFailed)?;

for transaction in dma_transactions {
if transaction.is_bounce_buffer {
// Code to release bounce buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need copy out from bounce buffer here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The caller may not know if it's bounce buffer or not, so I think we need handle it here. And need pass the Memory range to copy data out. Actually, I think we need know the IO direction to decide which copy is needed (including the one in map_dma_ranges).

} else if transaction.is_pinned && !transaction.is_prepinned {
// Code to unpin memory
}
}
Ok(())
}
}


// Implementation of the DMA interface for `DmaClient`
impl DmaInterface for DmaClient {
fn map_dma_ranges(&self, ranges: &[MemoryRange]) -> Result<DmaTransactionHandler, DmaError> {
self.map_dma_ranges(ranges)
}

fn unmap_dma_ranges(&self, dma_transactions: &[DmaTransaction]) -> Result<(), DmaError> {
self.unmap_dma_ranges(dma_transactions)
}
}
85 changes: 85 additions & 0 deletions openhcl/underhill_core/src/dma_manager.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
use std::sync::{Arc, Mutex, Weak};
use memory_range::MemoryRange;
use once_cell::sync::OnceCell;

pub use dma_client::{DmaClient, DmaInterface, DmaTransaction, DmaTransactionHandler};

pub enum DmaError {
InitializationFailed,
MapFailed,
UnmapFailed,
PinFailed,
BounceBufferFailed,
}

static GLOBAL_DMA_MANAGER: OnceCell<Arc<GlobalDmaManager>> = OnceCell::new();

/// Global DMA Manager to handle resources and manage clients
pub struct GlobalDmaManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

What settings will this manager have? Which of those do you expect to expose in Vtl2Settings?

physical_ranges: Vec<MemoryRange>,
bounce_buffers: Vec<MemoryRange>,
clients: Mutex<Vec<Weak<DmaClient>>>,
client_thresholds: Mutex<Vec<(Weak<DmaClient>, usize)>>,
}

impl GlobalDmaManager {
/// Initializes the global DMA manager with physical ranges and bounce buffers
pub fn initialize(
physical_ranges: Vec<MemoryRange>,
bounce_buffers: Vec<MemoryRange>,
) -> Result<(), DmaError> {
let manager = Arc::new(Self {
physical_ranges,
bounce_buffers,
clients: Mutex::new(Vec::new()),
client_thresholds: Mutex::new(Vec::new()),
});

GLOBAL_DMA_MANAGER.set(manager).map_err(|_| DmaError::InitializationFailed)
}

/// Accesses the singleton instance of the global manager
pub fn get_instance() -> Arc<GlobalDmaManager> {
GLOBAL_DMA_MANAGER
.get()
.expect("GlobalDmaManager has not been initialized")
.clone()
}

/// Creates a new `DmaClient` and registers it with the global manager, along with its threshold
pub fn create_client(&self, pinning_threshold: usize) -> Arc<DmaClient> {
let client = Arc::new(DmaClient::new(Arc::downgrade(&self.get_instance())));
self.register_client(&client, pinning_threshold);
client
}

/// Adds a new client to the list and stores its pinning threshold
fn register_client(&self, client: &Arc<DmaClient>, threshold: usize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to have per-client bounce buffers?

let mut clients = self.clients.lock().unwrap();
clients.push(Arc::downgrade(client));

let mut thresholds = self.client_thresholds.lock().unwrap();
thresholds.push((Arc::downgrade(client), threshold));
}

/// Retrieves the pinning threshold for a given client
pub fn get_client_threshold(&self, client: &Arc<DmaClient>) -> Option<usize> {
let thresholds = self.client_thresholds.lock().unwrap();
thresholds.iter().find_map(|(weak_client, threshold)| {
weak_client
.upgrade()
.filter(|c| Arc::ptr_eq(c, client))
.map(|_| *threshold)
})
}

/// Checks if the given memory range is already pinned
pub fn is_pinned(&self, range: &MemoryRange) -> bool {
false // Placeholder
}

/// Allocates a bounce buffer if available, otherwise returns an error
pub fn allocate_bounce_buffer(&self, size: usize) -> Result<usize, DmaError> {
Err(DmaError::BounceBufferFailed) // Placeholder
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need ensure the bounce buffer is page aligned?

Copy link
Author

Choose a reason for hiding this comment

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

I envision that bounce buffer management will be aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

And also note it: our current bounce buffer allocation function has infinite loop issue which we want to avoid it in the new implementation.

}
}
2 changes: 2 additions & 0 deletions openhcl/underhill_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ mod vp;
mod vpci;
mod worker;
mod wrapped_partition;
mod dma_manager;
mod dma_client;

// `pub` so that the missing_docs warning fires for options without
// documentation.
Expand Down
31 changes: 31 additions & 0 deletions vm/devices/user_driver/src/dma.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@

#[derive(Debug)]
pub enum DmaError {
InitializationFailed,
MapFailed,
UnmapFailed,
PinFailed,
BounceBufferFailed,
}

// Structure encapsulating the result of a DMA mapping operation
pub struct DmaTransactionHandler {
pub transactions: Vec<DmaTransaction>,
}

// Structure representing a DMA transaction with address and metadata
pub struct DmaTransaction {
pub original_addr: usize,
pub dma_addr: usize,
pub size: usize,
pub is_pinned: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have comments explaining these fields? like is_pinned and is_bounce_buffer cannot be both true, why do we need keep both?

Copy link
Author

Choose a reason for hiding this comment

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

yes, i will add it.

pub is_bounce_buffer: bool,
pub is_physical: bool,
pub is_prepinned: bool,
}

// Trait for the DMA interface
pub trait DmaInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you envision that this would replace other uses of bounce buffering? (for example, copying from private memory into shared memory for isolated VMs, or when the block disk bounces for arm64 guests)?

Copy link
Author

Choose a reason for hiding this comment

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

yes, i do envision that.
Policy on GlobalDmaManager can be control the behavior system wide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. How do you envision handling the case:

  1. Have a VTL0 VM where sometimes memory needs to be pinned.
  2. This particular transaction's memory must be placed in a bounce buffer, even if pinning would otherwise succeed?

(I'm thinking about the block device driver here, where it would never want to pin memory - the kernel doesn't know about the VTL0 addresses.)

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this offline. map_dma_ranges will take additional per-transaction parameters. For example, some clients may want all transactions to be placed into the bounce buffer.

fn map_dma_ranges(&self, ranges: &[MemoryRange]) -> Result<DmaTransactionHandler, DmaError>;
fn unmap_dma_ranges(&self, dma_transactions: &[DmaTransaction]) -> Result<(), DmaError>;
}
1 change: 1 addition & 0 deletions vm/devices/user_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub mod interrupt;
pub mod lockmem;
pub mod memory;
pub mod vfio;
pub mod dma;

pub type DmaAllocator<T> = <T as DeviceBacking>::DmaAllocator;

Expand Down
Loading