RFE: Skeleton for DMA layer #306

bhargavshah1988 · 2024-11-12T23:48:38Z

Design Doc : https://microsoft.sharepoint.com/:w:/t/osg_core_bnb/hyp/Eea7j8SZN-tEmDrG_RS3aO8BEOhIluPc4hDJOKj7oX1Zpw?e=Q7e5Dh

mattkur · 2024-11-13T00:04:56Z

openhcl/underhill_core/src/dma_client.rs

+        }
+    }
+
+    pub fn map_dma_ranges(


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

mattkur · 2024-11-13T00:06:08Z

openhcl/underhill_core/src/dma_manager.rs

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


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

mattkur · 2024-11-13T00:08:37Z

vm/devices/user_driver/src/dma.rs

+}
+
+// Trait for the DMA interface
+pub trait DmaInterface {


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)?

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

Got it. How do you envision handling the case:

Have a VTL0 VM where sometimes memory needs to be pinned.

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.)

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.

mattkur · 2024-11-14T18:13:19Z

openhcl/underhill_core/src/dma_manager.rs

+static GLOBAL_DMA_MANAGER: OnceCell<Arc<GlobalDmaManager>> = OnceCell::new();
+
+/// Global DMA Manager to handle resources and manage clients
+pub struct GlobalDmaManager {


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

…988/openvmm into user/bhsh/dma_layer

juantian8seattle · 2025-01-06T21:14:33Z

openhcl/underhill_core/src/dma_client.rs

+        let mut dma_transactions = Vec::new();
+        let force_bounce_buffer = options.map_or(false, |opts| opts.force_bounce_buffer);
+
+        let threshold = manager.get_client_threshold(self).ok_or(DmaError::InitializationFailed)?;


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

juantian8seattle · 2025-01-06T21:15:20Z

openhcl/underhill_core/src/dma_client.rs

+
+        for range in ranges {
+            let use_bounce_buffer = force_bounce_buffer || range_size > threshold || !self.can_pin(range);
+


extra empty line

juantian8seattle · 2025-01-06T21:19:07Z

openhcl/underhill_core/src/dma_client.rs

+
+        for transaction in dma_transactions {
+            if transaction.is_bounce_buffer {
+                // Code to release bounce buffer


Do we need copy out from bounce buffer here?

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).

juantian8seattle · 2025-01-06T21:25:49Z

openhcl/underhill_core/src/dma_manager.rs

+
+    /// 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


Do we need ensure the bounce buffer is page aligned?

I envision that bounce buffer management will be aligned.

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

juantian8seattle · 2025-01-06T21:29:28Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+        let result = self.issue_raw(command).await;
+
+        dma_client
+            .unmap_dma_ranges(&dma_transactions.transactions)


So we need handle the same functionality as copy_to_guest_memory in unmap_dma_ranges if opcode.transfer_controller_to_host()

juantian8seattle · 2025-01-06T21:34:59Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+        let result = self.issue_raw(command).await;
+
+        dma_client
+            .unmap_dma_ranges(&dma_transactions.transactions)


To simplify usage, can we just pass dma_transactions to unmap_dma_ranges? So caller needn't know details of DmaTransactionHandler?

juantian8seattle · 2025-01-06T21:37:44Z

vm/devices/user_driver/src/dma.rs

+    pub original_addr: usize,
+    pub dma_addr: usize,
+    pub size: usize,
+    pub is_pinned: bool,


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

yes, i will add it.

Agree with Juan. It seems you can go further, and do something like:

pub enum MemoryBacking { Pinned(prepinned: bool), InBounceBuffer }

And rather than doing if x.is_pinned, instead do match x.backing { Pinned => ...

if dma mapping options are disjoint (ie pinned or is bounce buffer), then they should be represented with an enum like Matt suggested.

Yes, will change this to enum.

juantian8seattle · 2025-01-07T17:29:08Z

openhcl/underhill_core/src/dma_client.rs

+        let threshold = manager.get_client_threshold(self).ok_or(DmaError::InitializationFailed)?;
+
+        for range in ranges {
+            let use_bounce_buffer = force_bounce_buffer || range_size > threshold || !self.can_pin(range);


If can_pin returns true, would it grantee the later pin_memory succeed?
I guess not. Then what's the strategy to handle pin/bounce buffer failures?
I mean, should we fallback to bounce buffer if pin failed?
Should we try pin if bounce buffer allocation failed?

mattkur · 2025-01-10T20:17:21Z

openhcl/underhill_core/src/dma_client.rs

+        Ok(DmaTransactionHandler { transactions })
+    }
+
+    pub fn unmap_dma_ranges(&self, dma_transactions: &[DmaTransaction]) -> Result<(), DmaError> {


Will this need to be an &mut reference to the dma_transactions?

mattkur · 2025-01-10T20:20:41Z

openhcl/underhill_core/src/dma_manager.rs

+use memory_range::MemoryRange;
+use once_cell::sync::OnceCell;
+
+pub use dma_client::{DmaClient, DmaInterface, DmaTransaction, DmaTransactionHandler};


I think either clippy or fmt will want you to split these out. Doesn't hurt to run cargo xtask fmt --fix on your code just to avoid folks noticing this kind of stuff.

mattkur · 2025-01-10T20:21:43Z

openhcl/underhill_core/src/dma_manager.rs

+    MapFailed,
+    UnmapFailed,
+    PinFailed,
+    BounceBufferFailed,


Please use source attributes so that we don't lose error origination. E.g.

#[derive(Error, Debug)] pub enum DmaError { ... PinFailed(#[source] ... error type)

mattkur · 2025-01-10T20:37:28Z

Please update this pr description with a high level overview of what's going on (including any design choices that you are making, and other options considered but not implemented & why).

As we get past the draft stage, this code will also need tests. But I appreciate having the design dialog via code - thanks!

mattkur · 2025-01-10T20:41:24Z

High level question: this machinery needs to work across a save & restore (e.g. an nvme device can have outstanding IO across an openhcl servicing operation). Have you yet considered how this would plug in to that?

bhargavshah1988 · 2025-01-10T22:44:21Z

High level question: this machinery needs to work across a save & restore (e.g. an nvme device can have outstanding IO across an openhcl servicing operation). Have you yet considered how this would plug in to that?

@mattkur DMA manager will save its self. However, in flight transection and client needs to save and restore by consumer(NVMe/MANA).
Do you agree ?

mattkur · 2025-01-13T17:37:39Z

High level question: this machinery needs to work across a save & restore (e.g. an nvme device can have outstanding IO across an openhcl servicing operation). Have you yet considered how this would plug in to that?

@mattkur DMA manager will save its self. However, in flight transection and client needs to save and restore by consumer(NVMe/MANA). Do you agree ?

Sure. We will need some mechanism to:

save the DmaTransaction objects themselves (e.g. they need to have a stable save state defined), and/or
reconstruct the state. E.g. let's say there are dma buffers that need to be saved/restored ... what is the API by which the devices hook up the save state so that the right thing happens when IOs complete

chris-oo · 2025-01-13T19:21:49Z

vm/devices/user_driver/src/dma.rs

+// Trait for the DMA interface
+pub trait DmaInterface {
+    fn map_dma_ranges(&self, ranges: &[MemoryRange], options: Option<&DmaMapOptions>,) -> Result<DmaTransactionHandler, DmaError>;
+    fn unmap_dma_ranges(&self, dma_transactions: &[DmaTransaction]) -> Result<(), DmaError>;


Not returning an opaque handle but asking the caller to provide some pub struct fields seems a bit odd to me, but we can always iterate on this api later since all users will be in-tree.

At least, it seems like perhaps map should return something opaque which also you can get the associated info that was mapped. I don't quite remember - we'd expect us to unmap the whole map call right? The way this is specified, a caller could just unmap a portion of the map call, is that what we want?

we'd expect us to unmap the whole map call right?
yes

The way this is specified, a caller could just unmap a portion of the map call, is that what we want?
So unmap can process multiple mapped transection at one go.

bhargavshah1988 · 2025-01-13T19:38:46Z

High level question: this machinery needs to work across a save & restore (e.g. an nvme device can have outstanding IO across an openhcl servicing operation). Have you yet considered how this would plug in to that?

@mattkur DMA manager will save its self. However, in flight transection and client needs to save and restore by consumer(NVMe/MANA). Do you agree ?

Sure. We will need some mechanism to:

save the DmaTransaction objects themselves (e.g. they need to have a stable save state defined), and/or

reconstruct the state. E.g. let's say there are dma buffers that need to be saved/restored ... what is the API by which the devices hook up the save state so that the right thing happens when IOs complete

The bounce buffer will be created from persistent memory across the UH servicing. Also the command and completion queues will be allocated from the persistent memory.
Drivers/DMA manager will save and restore pointers to these queues and buffer.

On restore, NVMe driver will restore the transection/completion the way its doing today.

DmaTransaction needs to be saved by NVMe/MANA so on restore size it can reconnect and replenish the transection.

bhargavshah1988 added 2 commits November 12, 2024 13:10

Merge branch 'main' of https://github.com/bhargavshah1988/openvmm

57c366b

RFE: Skeleton for DMA layer

d991f9e

mattkur reviewed Nov 13, 2024

View reviewed changes

mattkur assigned mattkur, jstarks and juantian8seattle and unassigned mattkur Nov 13, 2024

mattkur reviewed Nov 14, 2024

View reviewed changes

bhargavshah1988 added 4 commits December 23, 2024 10:25

Merge branch 'main' of https://github.com/bhargavshah1988/openvmm

2f4ad18

RFE: Skeleton for DMA layer

3163d08

Merge branch 'user/bhsh/dma_layer' of https://github.com/bhargavshah1…

728ac70

…988/openvmm into user/bhsh/dma_layer

PoC: storage driver change

eb7efbc

juantian8seattle reviewed Jan 6, 2025

View reviewed changes

juantian8seattle reviewed Jan 7, 2025

View reviewed changes

mattkur reviewed Jan 10, 2025

View reviewed changes

bhargavshah1988 closed this Jan 10, 2025

chris-oo reopened this Jan 13, 2025

chris-oo reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFE: Skeleton for DMA layer #306

RFE: Skeleton for DMA layer #306

bhargavshah1988 commented Nov 12, 2024 •

edited

Loading

mattkur Nov 13, 2024

mattkur Nov 13, 2024

mattkur Nov 13, 2024

bhargavshah1988 Nov 13, 2024

mattkur Nov 13, 2024

mattkur Nov 14, 2024

mattkur Nov 14, 2024

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

bhargavshah1988 Jan 6, 2025

juantian8seattle Jan 7, 2025

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

juantian8seattle Jan 6, 2025

bhargavshah1988 Jan 6, 2025

mattkur Jan 10, 2025 •

edited

Loading

chris-oo Jan 13, 2025

bhargavshah1988 Jan 13, 2025

juantian8seattle Jan 7, 2025

mattkur Jan 10, 2025

mattkur Jan 10, 2025

mattkur Jan 10, 2025

mattkur commented Jan 10, 2025

mattkur commented Jan 10, 2025

bhargavshah1988 commented Jan 10, 2025

mattkur commented Jan 13, 2025

chris-oo Jan 13, 2025

bhargavshah1988 Jan 13, 2025

bhargavshah1988 commented Jan 13, 2025


		for range in ranges {
		let use_bounce_buffer = force_bounce_buffer \|\| range_size > threshold \|\| !self.can_pin(range);

RFE: Skeleton for DMA layer #306

Are you sure you want to change the base?

RFE: Skeleton for DMA layer #306

Conversation

bhargavshah1988 commented Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkur Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkur commented Jan 10, 2025

mattkur commented Jan 10, 2025

bhargavshah1988 commented Jan 10, 2025

mattkur commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhargavshah1988 commented Jan 13, 2025

bhargavshah1988 commented Nov 12, 2024 •

edited

Loading

mattkur Jan 10, 2025 •

edited

Loading