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

Bring back DAX #205

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions src/arch/src/aarch64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ pub enum Error {

/// The start of the memory area reserved for MMIO devices.
pub const MMIO_MEM_START: u64 = layout::MAPPED_IO_START;
/// The size of the MMIO shared memory area used by virtio-fs DAX.
pub const MMIO_SHM_SIZE: u64 = 1 << 33;
/// The size of the MMIO shared memory area used by virtio-fs DAX and virtio-gpu.
pub const MMIO_SHM_SIZE: u64 = 1 << 34;

pub use self::fdt::DeviceInfoForFDT;
use crate::DeviceType;
Expand Down
4 changes: 2 additions & 2 deletions src/arch/src/x86_64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ const FIRST_ADDR_PAST_32BITS: u64 = 1 << 32;
const MEM_32BIT_GAP_SIZE: u64 = 768 << 20;
/// The start of the memory area reserved for MMIO devices.
pub const MMIO_MEM_START: u64 = FIRST_ADDR_PAST_32BITS - MEM_32BIT_GAP_SIZE;
/// The size of the MMIO shared memory area used by virtio-fs DAX.
pub const MMIO_SHM_SIZE: u64 = 1 << 33;
/// The size of the MMIO shared memory area used by virtio-fs DAX and virtio-gpu.
pub const MMIO_SHM_SIZE: u64 = 1 << 34;

/// Returns a Vec of the valid memory addresses.
/// These should be used to configure the GuestMemoryMmap structure for the platform.
Expand Down
1 change: 1 addition & 0 deletions src/devices/src/virtio/fs/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ impl VirtioDevice for Fs {
mem.clone(),
self.passthrough_cfg.clone(),
self.worker_stopfd.try_clone().unwrap(),
self.shm_region.clone(),
);
self.worker_thread = Some(worker.run());

Expand Down
9 changes: 8 additions & 1 deletion src/devices/src/virtio/fs/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use super::descriptor_utils::{Reader, Writer};
use super::passthrough::{self, PassthroughFs};
use super::server::Server;
use crate::legacy::Gic;
use crate::virtio::VirtioShmRegion;

pub struct FsWorker {
queues: Vec<Queue>,
Expand All @@ -25,6 +26,7 @@ pub struct FsWorker {
mem: GuestMemoryMmap,
server: Server<PassthroughFs>,
stop_fd: EventFd,
shm_region: Option<VirtioShmRegion>,
}

impl FsWorker {
Expand All @@ -39,6 +41,7 @@ impl FsWorker {
mem: GuestMemoryMmap,
passthrough_cfg: passthrough::Config,
stop_fd: EventFd,
shm_region: Option<VirtioShmRegion>,
) -> Self {
Self {
queues,
Expand All @@ -51,6 +54,7 @@ impl FsWorker {
mem,
server: Server::new(PassthroughFs::new(passthrough_cfg).unwrap()),
stop_fd,
shm_region,
}
}

Expand Down Expand Up @@ -149,7 +153,10 @@ impl FsWorker {
.map_err(FsError::QueueWriter)
.unwrap();

if let Err(e) = self.server.handle_message(reader, writer, None) {
if let Err(e) = self
.server
.handle_message(reader, writer, self.shm_region.as_ref())
{
error!("error handling message: {:?}", e);
}

Expand Down
22 changes: 18 additions & 4 deletions src/vmm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -554,13 +554,27 @@ pub fn build_microvm(
)?;
}

// Use half of the shmem region for virtio-gpu and half for DAX
let _shm_region_size = arch_memory_info.shm_size >> 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

My rust is still very recent: should the underscore be removed, here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That silences the unused variable warning, in the case of a build with the tee feature enabled and the gpu feature disabled (which would cause it to become unused).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the noise!


#[cfg(not(feature = "tee"))]
let _shm_region = Some(VirtioShmRegion {
let dax_shm_region = Some(VirtioShmRegion {
host_addr: guest_memory
.get_host_address(GuestAddress(arch_memory_info.shm_start_addr))
.unwrap() as u64,
guest_addr: arch_memory_info.shm_start_addr,
size: arch_memory_info.shm_size as usize,
size: _shm_region_size as usize,
});

#[cfg(feature = "gpu")]
let gpu_shm_region = Some(VirtioShmRegion {
host_addr: guest_memory
.get_host_address(GuestAddress(
arch_memory_info.shm_start_addr + _shm_region_size,
))
.unwrap() as u64,
guest_addr: arch_memory_info.shm_start_addr + _shm_region_size,
size: _shm_region_size as usize,
});

let mut vmm = Vmm {
Expand Down Expand Up @@ -591,15 +605,15 @@ pub fn build_microvm(
attach_gpu_device(
&mut vmm,
event_manager,
_shm_region,
gpu_shm_region,
intc.clone(),
virgl_flags,
#[cfg(target_os = "macos")]
_map_sender,
)?;
}
#[cfg(not(feature = "tee"))]
attach_fs_devices(&mut vmm, &vm_resources.fs, None, intc.clone())?;
attach_fs_devices(&mut vmm, &vm_resources.fs, dax_shm_region, intc.clone())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the feeling, from what I have tested, that this share memory region can't be used by more than one virtio device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work fine for me? I don't see why it wouldn't work as long as the range used by each device does not overlap (which is why I split it in half here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the half/half splitting between gpu and fs clearly works.
If the user has two virtiofs devices, they end up sharing the exact same portion, though.
That makes all my virtiofs devices fail to setup, except for the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, sorry, I misunderstood. Maybe @slp knows what the intent was here? I only brought back the previous behavior from back when DAX was enabled, so I'm guessing this same problem already existed before?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to implement some kind of shm region manager that manages the window and can be used to request regions of a certain size. I'm adding this to my TODO list and I think I'll have bandwidth to implement it next week, in case no one beats me implementing it first.

#[cfg(feature = "blk")]
attach_block_devices(&mut vmm, &vm_resources.block, intc.clone())?;
if let Some(vsock) = vm_resources.vsock.get() {
Expand Down
2 changes: 1 addition & 1 deletion src/vmm/src/vmm_config/boot_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::fmt::{Display, Formatter, Result};

#[cfg(all(target_os = "linux", not(feature = "tee")))]
pub const DEFAULT_KERNEL_CMDLINE: &str = "reboot=k panic=-1 panic_print=0 nomodule console=hvc0 \
rootfstype=virtiofs rw quiet no-kvmapf";
rootflags=dax rootfstype=virtiofs rw quiet no-kvmapf";

#[cfg(feature = "amd-sev")]
pub const DEFAULT_KERNEL_CMDLINE: &str = "reboot=k panic=-1 panic_print=0 nomodule console=hvc0 \
Expand Down
Loading