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.

Sign up for GitHub

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

Jump to bottom

shared_pool_alloc: zero out allocated DMA buffers #267

Merged
merged 2 commits into from
Nov 8, 2024
Merged

shared_pool_alloc: zero out allocated DMA buffers #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a1d106
Zero out allocated DMA buffers
eric135 Nov 7, 2024
e97d75b
Fix VfioDmaBuffer comment about zero-initialization
eric135 Nov 8, 2024
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
5 changes: 5 additions & 0 deletions openhcl/shared_pool_alloc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ impl user_driver::vfio::VfioDmaBuffer for SharedPoolAllocator {
.map_file(0, len, gpa_fd.get(), file_offset, true)
.context("unable to map allocation")?;

// It is a requirement of the VfioDmaBuffer trait that all allocated buffers be zeroed out
mapping
.fill_at(0, 0, len)
.context("failed to zero shared memory")?;

let pfns: Vec<_> = (alloc.base_pfn()..alloc.base_pfn() + alloc.size_pages).collect();

Ok(user_driver::memory::MemoryBlock::new(SharedDmaBuffer {
Expand Down
2 changes: 1 addition & 1 deletion vm/devices/user_driver/src/vfio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use zerocopy::AsBytes;
use zerocopy::FromBytes;

pub trait VfioDmaBuffer: 'static + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just sanity checking, there's nothing else that implements this trait today?

Copy link

Choose a reason for hiding this comment

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

Today it is LockedMemorySpawner and SharedPoolAllocator. Tomorrow also EmulatedDmaAllocator for unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do they all follow the new contract?

Copy link

Choose a reason for hiding this comment

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

This change benefits normal boot and all clients are compatible. For Restore functionality there is alternative path to restore buffers without zeroiniting them, at least how it's implemented in #210

After we're going to merge with #260 then I'll test end-to-end if it works too.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I asked though. We've added a new requirement to this method, we need to make sure all types that implement this trait are meeting the new requirement.

Copy link
Contributor

@mattkur mattkur Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

All prod types do (LockedMemorySpawner gets it by way of MAP_ANONYMOUS). It seems that the test code does not?

https://github.com/microsoft/openvmm/blob/04454925c9fdad1845b4450743b739e16d8a8b4d/vm/devices/user_driver/src/emulated.rs#L244C1-L250C2

calls:

openvmm/vm/devices/user_driver/src/emulated.rs

Line 168 in 0445492

pub fn alloc(&self, len: usize) -> Option<DmaBuffer> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #299

/// Create a new dma buffer with the given `len` in bytes.
/// Create a new DMA buffer of the given `len` bytes. Guaranteed to be zero-initialized.
fn create_dma_buffer(&self, len: usize) -> anyhow::Result<MemoryBlock>;
}

Expand Down