From e1cc702327f5b7fe50e0d90a72b838136af53504 Mon Sep 17 00:00:00 2001 From: Anatol Belski Date: Thu, 8 Apr 2021 20:05:38 +0200 Subject: [PATCH] memory_manager: Fix address range calculation in MemorySlot The MCRS method returns a 64-bit memory range descriptor. The calculation is supposed to be done as follows: max = min + len - 1 However, every operand is represented not as a QWORD but as combination of two DWORDs for high and low part. Till now, the calculation was done this way, please see also inline comments: max.lo = min.lo + len.lo //this may overflow, need to carry over to high max.hi = min.hi + len.hi max.hi = max.hi - 1 // subtraction needs to happen on the low part This calculation has been corrected the following way: max.lo = min.lo + len.lo max.hi = min.hi + len.hi + (max.lo < min.lo) // check for overflow max.lo = max.lo - 1 // subtract from low part The relevant part from the generated ASL for the MCRS method: ``` Method (MCRS, 1, Serialized) { Acquire (MLCK, 0xFFFF) \_SB.MHPC.MSEL = Arg0 Name (MR64, ResourceTemplate () { QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite, 0x0000000000000000, // Granularity 0x0000000000000000, // Range Minimum 0xFFFFFFFFFFFFFFFE, // Range Maximum 0x0000000000000000, // Translation Offset 0xFFFFFFFFFFFFFFFF, // Length ,, _Y00, AddressRangeMemory, TypeStatic) }) CreateQWordField (MR64, \_SB.MHPC.MCRS._Y00._MIN, MINL) // _MIN: Minimum Base Address CreateDWordField (MR64, 0x12, MINH) CreateQWordField (MR64, \_SB.MHPC.MCRS._Y00._MAX, MAXL) // _MAX: Maximum Base Address CreateDWordField (MR64, 0x1A, MAXH) CreateQWordField (MR64, \_SB.MHPC.MCRS._Y00._LEN, LENL) // _LEN: Length CreateDWordField (MR64, 0x2A, LENH) MINL = \_SB.MHPC.MHBL MINH = \_SB.MHPC.MHBH LENL = \_SB.MHPC.MHLL LENH = \_SB.MHPC.MHLH MAXL = (MINL + LENL) /* \_SB_.MHPC.MCRS.LENL */ MAXH = (MINH + LENH) /* \_SB_.MHPC.MCRS.LENH */ If ((MAXL < MINL)) { MAXH += One /* \_SB_.MHPC.MCRS.MAXH */ } MAXL -= One Release (MLCK) Return (MR64) /* \_SB_.MHPC.MCRS.MR64 */ } ``` Fixes #1800. Signed-off-by: Anatol Belski --- docs/windows.md | 2 + tests/integration.rs | 104 ++++++++++++++++++++++++++++++++++++++ vmm/src/memory_manager.rs | 12 ++++- 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/docs/windows.md b/docs/windows.md index 0d4835ca3..0180fa125 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -188,6 +188,8 @@ This allows for SSH login from a remote machine, for example through the `admini CPU hotplug is supported. The VM operating system needs to support hotplug and be appropriately licensed. SKU limitations like constraints on the number of cores are to be taken into consideration. Note, that Windows doesn't support CPU hot-remove. When `ch-remote` is invoked to reduce the number of CPUs, the result will be visible after the OS reboot within the same hypervisor instance. +RAM hotplug is supported. Note, that while the `pnpmem.sys` driver in use supports RAM hot-remove, the RAM being unplugged has to be not in use and have no reserved pages. In most cases it means, hot-remove won't work. Same as with the CPU hot-remove, when `ch-remote` is invoked to reduce the RAM size, the result will be visible after the OS reboot. + ## Debugging The Windows guest debugging process relies heavily on QEMU and [socat](http://www.dest-unreach.org/socat/). The procedure requires two Windows VMs: diff --git a/tests/integration.rs b/tests/integration.rs index 36783a5d1..f7fee3734 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -5574,6 +5574,20 @@ mod tests { .unwrap_or(0); } + fn get_ram_size_windows(auth: &PasswordAuth) -> usize { + return ssh_command_ip_with_auth( + "powershell -Command \"(Get-CimInstance win32_computersystem).TotalPhysicalMemory\"", + &auth, + "192.168.249.2", + DEFAULT_SSH_RETRIES, + DEFAULT_SSH_TIMEOUT, + ) + .unwrap() + .trim() + .parse::() + .unwrap_or(0); + } + fn get_vcpu_threads_count(pid: u32) -> u8 { // ps -T -p 12345 | grep vcpu | wc -l let out = Command::new("ps") @@ -5821,6 +5835,96 @@ mod tests { handle_child_output(r, &output); } + + #[test] + fn test_windows_guest_ram_hotplug() { + let mut windows = WindowsDiskConfig::new(WINDOWS_IMAGE_NAME.to_string()); + let guest = Guest::new(&mut windows); + + let tmp_dir = TempDir::new_with_prefix("/tmp/ch").unwrap(); + let mut workload_path = dirs::home_dir().unwrap(); + workload_path.push("workloads"); + + let mut ovmf_path = workload_path.clone(); + ovmf_path.push(OVMF_NAME); + + let mut osdisk_path = workload_path; + osdisk_path.push(WINDOWS_IMAGE_NAME.to_string()); + + let api_socket = temp_api_path(&tmp_dir); + + let mut child = GuestCommand::new(&guest) + .args(&["--api-socket", &api_socket]) + .args(&["--cpus", "boot=2,kvm_hyperv=on"]) + .args(&["--memory", "size=2G,hotplug_size=5G"]) + .args(&["--kernel", ovmf_path.to_str().unwrap()]) + .default_disks() + .args(&["--serial", "tty"]) + .args(&["--console", "off"]) + .args(&["--net", "tap="]) + .capture_output() + .spawn() + .unwrap(); + + // Wait to make sure Windows boots up + thread::sleep(std::time::Duration::new(60, 0)); + + let r = std::panic::catch_unwind(|| { + let auth = windows_auth(); + + let ram_size = 2 * 1024 * 1024 * 1024; + // Check the initial number of RAM the guest sees + let current_ram_size = get_ram_size_windows(&auth); + // This size seems to be reserved by the system and thus the + // reported amount differs by this constant value. + let reserved_ram_size = ram_size - current_ram_size; + // Verify that there's not more than 4mb constant diff wasted + // by the reserved ram. + assert!(reserved_ram_size < 4 * 1024 * 1024); + + let ram_size = 4 * 1024 * 1024 * 1024; + // Hotplug some RAM + resize_command(&api_socket, None, Some(ram_size), None); + // Wait to make sure RAM has been added + thread::sleep(std::time::Duration::new(10, 0)); + // Check the guest sees the correct number + assert_eq!(get_ram_size_windows(&auth), ram_size - reserved_ram_size); + + let ram_size = 3 * 1024 * 1024 * 1024; + // Unplug some RAM. Note that hot-remove most likely won't work. + resize_command(&api_socket, None, Some(ram_size), None); + // Wait to make sure RAM has been added + thread::sleep(std::time::Duration::new(10, 0)); + // Reboot to let Windows catch up + ssh_command_ip_with_auth( + "shutdown /r /t 0", + &auth, + "192.168.249.2", + DEFAULT_SSH_RETRIES, + DEFAULT_SSH_TIMEOUT, + ) + .unwrap(); + // Wait to make sure guest completely rebooted + thread::sleep(std::time::Duration::new(60, 0)); + // Check the guest sees the correct number + assert_eq!(get_ram_size_windows(&auth), ram_size - reserved_ram_size); + + ssh_command_ip_with_auth( + "shutdown /s", + &auth, + "192.168.249.2", + DEFAULT_SSH_RETRIES, + DEFAULT_SSH_TIMEOUT, + ) + .unwrap(); + }); + + let _ = child.wait_timeout(std::time::Duration::from_secs(60)); + let _ = child.kill(); + let output = child.wait_with_output().unwrap(); + + handle_child_output(r, &output); + } } #[cfg(target_arch = "x86_64")] diff --git a/vmm/src/memory_manager.rs b/vmm/src/memory_manager.rs index 09a3c9a01..9f62a14fc 100644 --- a/vmm/src/memory_manager.rs +++ b/vmm/src/memory_manager.rs @@ -1810,9 +1810,17 @@ impl Aml for MemoryMethods { &aml::Path::new("MINH"), &aml::Path::new("LENH"), ), + &aml::If::new( + &aml::LessThan::new(&aml::Path::new("MAXL"), &aml::Path::new("MINL")), + vec![&aml::Add::new( + &aml::Path::new("MAXH"), + &aml::ONE, + &aml::Path::new("MAXH"), + )], + ), &aml::Subtract::new( - &aml::Path::new("MAXH"), - &aml::Path::new("MAXH"), + &aml::Path::new("MAXL"), + &aml::Path::new("MAXL"), &aml::ONE, ), // Release lock