From 1df38daf749d671a299a9895434307b1fe092b7b Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Fri, 24 Apr 2020 16:58:03 +0100 Subject: [PATCH] vmm, tests: Make specifying a size optional for virtio-pmem If a size is specified use it (in particular this is required if the destination is a directory) otherwise seek in the file to get the size of the file. Add a new check that the size is a multiple of 2MiB otherwise the kernel will reject it. Signed-off-by: Rob Bradford --- tests/integration.rs | 17 ++++--- vmm/src/api/openapi/cloud-hypervisor.yaml | 1 - vmm/src/config.rs | 19 +++----- vmm/src/device_manager.rs | 57 +++++++++++++++-------- 4 files changed, 55 insertions(+), 39 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index a394f1540..e321831f7 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1987,15 +1987,20 @@ mod tests { #[cfg_attr(not(feature = "mmio"), test)] fn test_virtio_pmem_persist_writes() { - test_virtio_pmem(false) + test_virtio_pmem(false, false) } #[cfg_attr(not(feature = "mmio"), test)] fn test_virtio_pmem_discard_writes() { - test_virtio_pmem(true) + test_virtio_pmem(true, false) } - fn test_virtio_pmem(discard_writes: bool) { + #[cfg_attr(not(feature = "mmio"), test)] + fn test_virtio_pmem_with_size() { + test_virtio_pmem(true, true) + } + + fn test_virtio_pmem(discard_writes: bool, specify_size: bool) { test_block!(tb, "", { let mut clear = ClearDiskConfig::new(); let guest = Guest::new(&mut clear); @@ -2020,9 +2025,9 @@ mod tests { .args(&[ "--pmem", format!( - "file={},size={}{}", + "file={}{}{}", pmem_temp_file.path().to_str().unwrap(), - "128M", + if specify_size { ",size=128M" } else { "" }, if discard_writes { ",discard_writes=on" } else { @@ -3836,7 +3841,7 @@ mod tests { &api_socket, "add-pmem", Some(&format!( - "file={},size=128M,id=test0", + "file={},id=test0", pmem_temp_file.path().to_str().unwrap() )) ) diff --git a/vmm/src/api/openapi/cloud-hypervisor.yaml b/vmm/src/api/openapi/cloud-hypervisor.yaml index 79d640fe1..f4287a454 100644 --- a/vmm/src/api/openapi/cloud-hypervisor.yaml +++ b/vmm/src/api/openapi/cloud-hypervisor.yaml @@ -514,7 +514,6 @@ components: PmemConfig: required: - file - - size type: object properties: file: diff --git a/vmm/src/config.rs b/vmm/src/config.rs index 10221b53d..2fedb61ec 100644 --- a/vmm/src/config.rs +++ b/vmm/src/config.rs @@ -34,8 +34,6 @@ pub enum Error { InvalidCacheSizeWithDaxOff, /// Missing persistant memory file parameter. ParsePmemFileMissing, - /// Missing persistant memory size parameter. - ParsePmemSizeMissing, /// Missing vsock socket path parameter. ParseVsockSockMissing, /// Missing vsock cid parameter. @@ -126,8 +124,6 @@ impl fmt::Display for Error { } ParsePersistentMemory(o) => write!(f, "Error parsing --pmem: {}", o), ParsePmemFileMissing => write!(f, "Error parsing --pmem: file missing"), - ParsePmemSizeMissing => write!(f, "Error parsing --pmem: size missing"), - ParseVsock(o) => write!(f, "Error parsing --vsock: {}", o), ParseVsockCidMissing => write!(f, "Error parsing --vsock: cid missing"), ParseVsockSockMissing => write!(f, "Error parsing --vsock: sock missing"), @@ -959,7 +955,8 @@ impl FsConfig { #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, Default)] pub struct PmemConfig { pub file: PathBuf, - pub size: u64, + #[serde(default)] + pub size: Option, #[serde(default)] pub iommu: bool, #[serde(default)] @@ -988,9 +985,8 @@ impl PmemConfig { let file = PathBuf::from(parser.get("file").ok_or(Error::ParsePmemFileMissing)?); let size = parser .convert::("size") - .map_err(Error::ParseMemory)? - .ok_or(Error::ParsePmemSizeMissing)? - .0; + .map_err(Error::ParsePersistentMemory)? + .map(|v| v.0); let mergeable = parser .convert::("mergeable") .map_err(Error::ParsePersistentMemory)? @@ -1756,13 +1752,12 @@ mod tests { fn test_pmem_parsing() -> Result<()> { // Must always give a file and size assert!(PmemConfig::parse("").is_err()); - assert!(PmemConfig::parse("file=/tmp/pmem").is_err()); assert!(PmemConfig::parse("size=128M").is_err()); assert_eq!( PmemConfig::parse("file=/tmp/pmem,size=128M")?, PmemConfig { file: PathBuf::from("/tmp/pmem"), - size: 128 << 20, + size: Some(128 << 20), ..Default::default() } ); @@ -1770,7 +1765,7 @@ mod tests { PmemConfig::parse("file=/tmp/pmem,size=128M,id=mypmem0")?, PmemConfig { file: PathBuf::from("/tmp/pmem"), - size: 128 << 20, + size: Some(128 << 20), id: Some("mypmem0".to_owned()), ..Default::default() } @@ -1779,7 +1774,7 @@ mod tests { PmemConfig::parse("file=/tmp/pmem,size=128M,iommu=on,mergeable=on,discard_writes=on")?, PmemConfig { file: PathBuf::from("/tmp/pmem"), - size: 128 << 20, + size: Some(128 << 20), mergeable: true, discard_writes: true, iommu: true, diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index e0ea04cd6..c09027815 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -39,7 +39,7 @@ use qcow::{self, ImageType, QcowFile}; use std::any::Any; use std::collections::HashMap; use std::fs::{File, OpenOptions}; -use std::io::{self, sink, stdout}; +use std::io::{self, sink, stdout, Seek, SeekFrom}; #[cfg(feature = "pci_support")] use std::num::Wrapping; use std::os::unix::fs::OpenOptionsExt; @@ -285,6 +285,12 @@ pub enum DeviceManagerError { /// Failed updating guest memory for VFIO PCI device. #[cfg(feature = "pci_support")] UpdateMemoryForVfioPciDevice(VfioPciError), + + /// Trying to use a directory for pmem but no size specified + PmemWithDirectorySizeMissing, + + /// Trying to use a size that is not multiple of 2MiB + PmemSizeNotAligned, } pub type DeviceManagerResult = result::Result; @@ -1522,7 +1528,36 @@ impl DeviceManager { pmem_cfg.id = self.next_device_name(PMEM_DEVICE_NAME_PREFIX)?; } - let size = pmem_cfg.size; + let (custom_flags, set_len) = if pmem_cfg.file.is_dir() { + if pmem_cfg.size.is_none() { + return Err(DeviceManagerError::PmemWithDirectorySizeMissing); + } + (O_TMPFILE, true) + } else { + (0, false) + }; + + let mut file = OpenOptions::new() + .read(true) + .write(!pmem_cfg.discard_writes) + .custom_flags(custom_flags) + .open(&pmem_cfg.file) + .map_err(DeviceManagerError::PmemFileOpen)?; + + let size = if let Some(size) = pmem_cfg.size { + if set_len { + file.set_len(size) + .map_err(DeviceManagerError::PmemFileSetLen)?; + } + size + } else { + file.seek(SeekFrom::End(0)) + .map_err(DeviceManagerError::PmemFileSetLen)? + }; + + if size % 0x20_0000 != 0 { + return Err(DeviceManagerError::PmemSizeNotAligned); + } // The memory needs to be 2MiB aligned in order to support // hugepages. @@ -1534,24 +1569,6 @@ impl DeviceManager { .allocate_mmio_addresses(None, size as GuestUsize, Some(0x0020_0000)) .ok_or(DeviceManagerError::PmemRangeAllocation)?; - let (custom_flags, set_len) = if pmem_cfg.file.is_dir() { - (O_TMPFILE, true) - } else { - (0, false) - }; - - let file = OpenOptions::new() - .read(true) - .write(!pmem_cfg.discard_writes) - .custom_flags(custom_flags) - .open(&pmem_cfg.file) - .map_err(DeviceManagerError::PmemFileOpen)?; - - if set_len { - file.set_len(size) - .map_err(DeviceManagerError::PmemFileSetLen)?; - } - let cloned_file = file.try_clone().map_err(DeviceManagerError::CloneFile)?; let mmap_region = MmapRegion::build( Some(FileOffset::new(cloned_file, 0)),