Use DMA framework to create DMAble buffers #722
Conversation
|
Please add more context in your PR description and update the title. |
bhargavshah1988
changed the title
| } | ||
| } | ||
|
|
||
| pub fn create_client(&mut self, pci_id: String) -> DmaClient { |
There was a problem hiding this comment.
I'm not sure we want to clone DmaClients, as it seems reasonable to me that there is at most one client per pci_id. I'd rather here we make the Arc external, ie don't implement clone, but instead return an Arc<DmaClient>
| self.clients.get(pci_id).cloned() | ||
| } | ||
|
|
||
| pub fn get_dma_buffer_allocator( |
There was a problem hiding this comment.
This doesn't make sense here - why is this not a method on DmaClient itself?
| save_restore_supported: bool, | ||
| saved_state: Option<NvmeSavedState>, | ||
| dma_manager: GlobalDmaManager, |
There was a problem hiding this comment.
Passing this by value raises questions as noted in the Clone derive for GlobalDmaManager.
openhcl/underhill_core/src/worker.rs
Outdated
| @@ -1863,43 +1872,67 @@ async fn new_underhill_vm( | |||
| crate::inspect_proc::periodic_telemetry_task(driver_source.simple()), | |||
| ); | |||
|
|
|||
| let dma_pool = if let Some(shared_vis_pages_pool) = shared_vis_pages_pool.clone() { | |||
There was a problem hiding this comment.
We cannot allow cloning the pools, for the reasons stated above. Which means, you'll need to refactor DmaManager to either take ownership of the pool, or have some method it can use to spawn allocators.
There was a problem hiding this comment.
You can probably temporarily get away with just taking the pool spawner to allow you to spawn allocators, but you cannot allow multiple instances of the pool around because then usage / allocation is broken (multiple users of the same GPA range)
| .device() | ||
| .host_allocator() | ||
|
|
||
| let dma_client = gdma.device().get_dma_client().context("Failed to get DMA client from device")?; |
There was a problem hiding this comment.
We talked about this before, but this get_dma_client should not return an Option, IE the method should not be faillable. Every device will want a dma client.
| pub async fn host_allocator(&self) -> DmaAllocator<T> { | ||
| self.inner.gdma.lock().await.device().host_allocator() | ||
| /// Returns an object that can allocate dma memory to be shared with the device. | ||
| pub async fn get_dma_client(&self) -> anyhow::Result<Arc<dyn DmaClient>> |
There was a problem hiding this comment.
prefer not using get_<thing> naming in Rust, generally just call it by it's name, so in this case dma_client like how previously it was host_dma_allocator.
| msix_info: IrqInfo, | ||
| #[inspect(skip)] | ||
| driver_source: VmTaskDriverSource, | ||
| #[inspect(iter_by_index)] | ||
| interrupts: Vec<Option<InterruptState>>, | ||
| #[inspect(skip)] | ||
| dma_client: Arc<dyn DmaClient>, |
There was a problem hiding this comment.
I think we should instead require DmaClient implement Inspect so that it's inspectable. You can imagine we'd might want to report stuff such as dma policy, outstanding dma allocations, etc.
| //clients: Mutex<Vec<Weak<DmaClient>>>, | ||
| //client_thresholds: Mutex<Vec<(Weak<DmaClient>, usize)>>, | ||
| dma_buffer_spawner: Box<dyn Fn(String) -> anyhow::Result<Arc<dyn VfioDmaBuffer>> + Send>, | ||
| clients: HashMap<String, Arc<DmaClient>>, |
There was a problem hiding this comment.
why do we need this clients hashmap?
| use memory_range::MemoryRange; | ||
| use std::{ | ||
| collections::HashMap, | ||
| sync::{Arc, Mutex}, |
| Ok(arc_client) // Return the `Arc<Mutex<DmaClient>>` | ||
| } | ||
|
|
||
| pub fn get_client(&self, pci_id: &str) -> Option<Arc<DmaClient>> { |
There was a problem hiding this comment.
again, why do we need this fn? shouldn't we just make a new client when requested?
| @@ -35,7 +35,8 @@ pub trait DeviceBacking: 'static + Send + Inspect { | |||
| /// Maps a BAR. | |||
| fn map_bar(&mut self, n: u8) -> anyhow::Result<Self::Registers>; | |||
|
|
|||
| fn get_dma_client(&self) -> Option<Arc<dyn DmaClient>>; | |||
| /// DMA Client for the device. | |||
| fn dma_client(&self) -> anyhow::Result<Arc<dyn DmaClient>>; | |||
There was a problem hiding this comment.
Again, this method should not be failable. It should just return Arc<dyn DmaClient>.
There was a problem hiding this comment.
Better yet, I would rather have it return DmaClient and have the associated type, like the code did before. Why did you make it an Arc? Let the caller decide if it needs to put it behind an Arc (but most probably will not).
There was a problem hiding this comment.
talked offline - confusingly VfioDevice itself uses dyn dispatch to allocate/restore dma buffers. I wonder if we want to remove the vtable dispatch here in the future.
Add support for DMA framework. Today This framework will only allocates the DMAble memory. But it will be extended to have pin/unpin and bounce buffer support.