Skip to content

disk_backend: add new layered disk #404

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

Merged
merged 11 commits into from
Nov 29, 2024
Merged

disk_backend: add new layered disk #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c953b85
disk_backend: add new layered disk
jstarks Nov 27, 2024
5a25d5b
misc
jstarks Nov 27, 2024
68bcf91
cleanup
jstarks Nov 27, 2024
41d93dd
resolvers
jstarks Nov 27, 2024
6b5121c
feedback,fix ram layer resize
jstarks Nov 27, 2024
24e1de0
clippy
jstarks Nov 27, 2024
f5bc7da
test unmap handling too
jstarks Nov 27, 2024
4bf6ce7
doc
jstarks Nov 27, 2024
6c296a0
move,remove zerodisk
jstarks Nov 27, 2024
6162575
whoops
jstarks Nov 27, 2024
c3dfe9d
feedback
jstarks Nov 28, 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
22 changes: 18 additions & 4 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,21 @@ dependencies = [
"vmgs_broker",
]

[[package]]
name = "disk_layered"
version = "0.0.0"
dependencies = [
"async-trait",
"disk_backend",
"disk_backend_resources",
"futures",
"guestmem",
"inspect",
"scsi_buffers",
"thiserror 2.0.0",
"vm_resource",
]

[[package]]
name = "disk_nvme"
version = "0.0.0"
Expand Down Expand Up @@ -1251,15 +1266,16 @@ name = "disk_ramdisk"
version = "0.0.0"
dependencies = [
"anyhow",
"async-trait",
"disk_backend",
"disk_backend_resources",
"disk_layered",
"event-listener",
"guestmem",
"inspect",
"pal_async",
"parking_lot",
"scsi_buffers",
"test_with_tracing",
"thiserror 2.0.0",
"tracing",
"vm_resource",
Expand Down Expand Up @@ -1993,7 +2009,6 @@ dependencies = [
"chipset_arc_mutex_device",
"chipset_device",
"chipset_device_fuzz",
"disk_backend",
"disk_ramdisk",
"guestmem",
"ide",
Expand Down Expand Up @@ -4197,7 +4212,6 @@ version = "0.0.0"
dependencies = [
"anyhow",
"chipset_device",
"disk_backend",
"disk_ramdisk",
"event-listener",
"futures",
Expand Down Expand Up @@ -4550,6 +4564,7 @@ dependencies = [
"disk_blob",
"disk_crypt",
"disk_file",
"disk_layered",
"disk_prwrap",
"disk_ramdisk",
"disk_vhd1",
Expand Down Expand Up @@ -5983,7 +5998,6 @@ dependencies = [
"anyhow",
"async-trait",
"criterion",
"disk_backend",
"disk_ramdisk",
"event-listener",
"fast_select",
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ disk_crypt = { path = "vm/devices/storage/disk_crypt" }
disk_crypt_resources = { path = "vm/devices/storage/disk_crypt_resources" }
disk_file = { path = "vm/devices/storage/disk_file" }
disk_get_vmgs = { path = "vm/devices/storage/disk_get_vmgs" }
disk_layered = { path = "vm/devices/storage/disk_layered" }
disk_nvme = { path = "vm/devices/storage/disk_nvme" }
disk_ramdisk = { path = "vm/devices/storage/disk_ramdisk" }
disk_striped = { path = "vm/devices/storage/disk_striped" }
Expand Down
27 changes: 20 additions & 7 deletions openvmm/openvmm_entry/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ use cli_args::EndpointConfigCli;
use cli_args::NicConfigCli;
use cli_args::SerialConfigCli;
use cli_args::UefiConsoleModeCli;
use disk_backend_resources::layer::DiskLayerHandle;
use disk_backend_resources::layer::RamDiskLayerHandle;
use floppy_resources::FloppyDiskConfig;
use framebuffer::FramebufferAccess;
use framebuffer::FRAMEBUFFER_SIZE;
Expand Down Expand Up @@ -844,9 +846,9 @@ fn vm_config_from_command_line(
let vmgs_disk = if let Some(disk) = &opt.get_vmgs {
disk_open(disk, false).context("failed to open GET vmgs disk")?
} else {
disk_backend_resources::RamDiskHandle {
len: vmgs_format::VMGS_DEFAULT_CAPACITY,
}
disk_backend_resources::LayeredDiskHandle::single_layer(RamDiskLayerHandle {
len: Some(vmgs_format::VMGS_DEFAULT_CAPACITY),
})
.into_resource()
};
vmbus_devices.extend([
Expand Down Expand Up @@ -1416,7 +1418,11 @@ impl NicConfig {

fn disk_open(disk_cli: &DiskCliKind, read_only: bool) -> anyhow::Result<Resource<DiskHandleKind>> {
let disk_type = match disk_cli {
&DiskCliKind::Memory(len) => Resource::new(disk_backend_resources::RamDiskHandle { len }),
&DiskCliKind::Memory(len) => {
Resource::new(disk_backend_resources::LayeredDiskHandle::single_layer(
RamDiskLayerHandle { len: Some(len) },
))
}
DiskCliKind::File(path) => open_disk_type(path, read_only)
.with_context(|| format!("failed to open {}", path.display()))?,
DiskCliKind::Blob { kind, url } => Resource::new(disk_backend_resources::BlobDiskHandle {
Expand All @@ -1427,8 +1433,13 @@ fn disk_open(disk_cli: &DiskCliKind, read_only: bool) -> anyhow::Result<Resource
},
}),
DiskCliKind::MemoryDiff(inner) => {
Resource::new(disk_backend_resources::RamDiffDiskHandle {
lower: disk_open(inner, true)?,
Resource::new(disk_backend_resources::LayeredDiskHandle {
layers: vec![
RamDiskLayerHandle { len: None }.into_resource().into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

to my eyes, I would've expected to see some kind of indication that the RamDiskLayer was implementing diff-disk semantics here (e.g: some sort of bool to configure the semantics). instead - it kinda just looks like you have a regular ram disk on-top of a underlying disk (i.e: a useless configuration)

Copy link
Member Author

Choose a reason for hiding this comment

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

A RAM disk layer starts out with all sectors "not present", not zero. When a read falls off the end of a layered disk (because the sector was not present in any of the layers), we zero out the sector.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I just need to get used to the different semantics that Layers have, vs. typical Disks

DiskLayerHandle(disk_open(inner, true)?)
.into_resource()
.into(),
],
})
}
DiskCliKind::PersistentReservationsWrapper(inner) => Resource::new(
Expand Down Expand Up @@ -2363,7 +2374,9 @@ async fn run_control(driver: &DefaultDriver, mesh: &VmmMesh, opt: Options) -> an
.with_context(|| format!("failed to open {}", path.display()))?
}
Some(size) => {
Resource::new(disk_backend_resources::RamDiskHandle { len: size })
Resource::new(disk_backend_resources::LayeredDiskHandle::single_layer(
RamDiskLayerHandle { len: Some(size) },
))
}
};

Expand Down
1 change: 1 addition & 0 deletions openvmm/openvmm_resources/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ serial_socket.workspace = true
disk_blob = { workspace = true, optional = true }
disk_crypt = { workspace = true, optional = true }
disk_file.workspace = true
disk_layered.workspace = true
disk_prwrap.workspace = true
disk_ramdisk.workspace = true
disk_vhd1.workspace = true
Expand Down
1 change: 1 addition & 0 deletions openvmm/openvmm_resources/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ vm_resource::register_static_resolvers! {
net_dio::resolver::DioResolver,

// Disks
disk_layered::resolver::LayeredDiskResolver,
#[cfg(feature = "disk_crypt")]
disk_crypt::resolver::DiskCryptResolver,
disk_ramdisk::resolver::RamDiskResolver,
Expand Down
36 changes: 27 additions & 9 deletions petri/src/vm/construct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use crate::BOOT_NVME_NSID;
use crate::SCSI_INSTANCE;
use crate::SIZE_1_GB;
use anyhow::Context;
use disk_backend_resources::RamDiffDiskHandle;
use disk_backend_resources::layer::DiskLayerHandle;
use disk_backend_resources::layer::RamDiskLayerHandle;
use disk_backend_resources::LayeredDiskHandle;
use framebuffer::Framebuffer;
use framebuffer::FramebufferAccess;
use framebuffer::FRAMEBUFFER_SIZE;
Expand Down Expand Up @@ -750,7 +752,13 @@ impl PetriVmConfigSetupCore<'_> {
PcatGuest::Vhd(_) => GuestMedia::Disk {
read_only: false,
disk_parameters: None,
disk_type: (RamDiffDiskHandle { lower: inner_disk }).into_resource(),
disk_type: LayeredDiskHandle {
layers: vec![
RamDiskLayerHandle { len: None }.into_resource().into(),
DiskLayerHandle(inner_disk).into_resource().into(),
],
}
.into_resource(),
},
PcatGuest::Iso(_) => GuestMedia::Dvd(
SimpleScsiDvdHandle {
Expand Down Expand Up @@ -790,8 +798,13 @@ impl PetriVmConfigSetupCore<'_> {
device: SimpleScsiDiskHandle {
read_only: false,
parameters: Default::default(),
disk: RamDiffDiskHandle {
lower: open_disk_type(&path, true)?,
disk: LayeredDiskHandle {
layers: vec![
RamDiskLayerHandle { len: None }.into_resource().into(),
DiskLayerHandle(open_disk_type(&path, true)?)
.into_resource()
.into(),
],
}
.into_resource(),
}
Expand All @@ -817,8 +830,13 @@ impl PetriVmConfigSetupCore<'_> {
msix_count: 64,
namespaces: vec![NamespaceDefinition {
nsid: BOOT_NVME_NSID,
disk: RamDiffDiskHandle {
lower: open_disk_type(&path, true)?,
disk: LayeredDiskHandle {
layers: vec![
RamDiskLayerHandle { len: None }.into_resource().into(),
DiskLayerHandle(open_disk_type(&path, true)?)
.into_resource()
.into(),
],
}
.into_resource(),
read_only: false,
Expand Down Expand Up @@ -925,9 +943,9 @@ impl PetriVmConfigSetupCore<'_> {
vmbus_redirection: false,
vtl2_settings: None, // Will be added at startup to allow tests to modify
vmgs_disk: Some(
disk_backend_resources::RamDiskHandle {
len: vmgs_format::VMGS_DEFAULT_CAPACITY,
}
LayeredDiskHandle::single_layer(RamDiskLayerHandle {
len: Some(vmgs_format::VMGS_DEFAULT_CAPACITY),
})
.into_resource(),
),
framebuffer: framebuffer.then(|| SharedFramebufferHandle.into_resource()),
Expand Down
6 changes: 1 addition & 5 deletions vm/devices/get/guest_emulation_device/src/test_utilities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use crate::GedChannel;
use crate::GuestConfig;
use crate::GuestEmulationDevice;
use crate::GuestFirmwareConfig;
use disk_backend::Disk;
use get_protocol::test_utilities::TEST_VMGS_CAPACITY;
use get_protocol::HostNotifications;
use get_protocol::HostRequests;
Expand Down Expand Up @@ -264,10 +263,7 @@ pub fn create_host_channel(
None,
recv,
None,
Some(
Disk::new(disk_ramdisk::RamDisk::new(TEST_VMGS_CAPACITY as u64, false).unwrap())
.unwrap(),
),
Some(disk_ramdisk::ram_disk(TEST_VMGS_CAPACITY as u64, false).unwrap()),
);

if let Some(ged_responses) = ged_responses {
Expand Down
7 changes: 1 addition & 6 deletions vm/devices/storage/disk_backend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,14 @@
//! (such as the VMGS file system).
//!
//! `Disk`s are backed by a [`DiskIo`] implementation. Specific disk
//! backends should be in their own crates. The exceptions that prove the rule
//! is [`ZeroDisk`][], which is small enough to be in this crate and serve as an
//! example.
//!
//! [`ZeroDisk`]: crate::zerodisk::ZeroDisk
//! backends should be in their own crates.

#![forbid(unsafe_code)]
#![warn(missing_docs)]

pub mod pr;
pub mod resolve;
pub mod sync_wrapper;
pub mod zerodisk;

use guestmem::AccessError;
use inspect::Inspect;
Expand Down
110 changes: 0 additions & 110 deletions vm/devices/storage/disk_backend/src/zerodisk.rs
View file
Open in desktop

This file was deleted.

Loading