WSLA: Switch VHD boot disk approach and support PMEM for VHD disks #13943
Conversation
OneBlue
left a comment
OneBlue
left a comment
There was a problem hiding this comment.
Change looks good, I'd be curious to see what the performance difference is with pmem vs scsi before making it the default though
Also do we know if there are versions of Windows that don't support it ?
There was a problem hiding this comment.
Pull request overview
This PR switches from using SCSI to PMEM for attaching the root VHD and kernel modules VHD that are known at VM boot time. The key benefits are: (1) device paths are known upfront without needing to query the guest, eliminating a round-trip communication step; (2) VHD attachment issues occur at VM creation time rather than during runtime, improving error detection.
Key Changes
- Added PMEM device structures to HCS schema (VirtualPMemDevice, VirtualPMemController, and related enums)
- Modified VM initialization to attach root and kernel modules VHDs via PMEM instead of SCSI
- Updated ConfigureMounts to accept device paths as parameters rather than discovering them via AttachDisk
- Consolidated security descriptor construction to use std::format for improved readability
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/windows/common/hcs_schema.h | Added PMEM device schema definitions (VirtualPMemDevice, VirtualPMemController, VirtualPMemImageFormat, VirtualPMemBackingType) and integrated them into the Devices struct |
| src/windows/wslaservice/exe/WSLAVirtualMachine.h | Updated ConfigureMounts signature to accept root and modules device paths as parameters |
| src/windows/wslaservice/exe/WSLAVirtualMachine.cpp | Implemented PMEM controller initialization, moved VHD attachment to VM creation time with deterministic device paths (/dev/pmem0, /dev/pmem1), and refactored ConfigureMounts to use provided device paths instead of AttachDisk calls |
benhillis
changed the title
| pmemController.MaximumCount = 0; | ||
| pmemController.MaximumSizeBytes = 0; | ||
| pmemController.Backing = hcs::VirtualPMemBackingType::Virtual; | ||
| auto attachPmemDisk = [&](PCWSTR path) { | ||
| ULONG deviceId = pmemController.MaximumCount; | ||
| pmemController.MaximumCount += 1; |
There was a problem hiding this comment.
The MaximumCount field is set to 0 initially and then incremented to track the number of devices, but it's typed as uint8_t in the schema. However, it's being used to represent a count that starts at 0 and is incremented. After attaching 2 devices (rootVhd and modulesVhd), MaximumCount becomes 2. This field name suggests it should represent the maximum number of devices allowed, not the current count. The field appears to be misnamed or misused - it should either be renamed to reflect its actual purpose (e.g., DeviceCount) or should be set to the maximum allowed count rather than the current count.
| auto getDevicePath = [&](std::variant<ULONG, std::string>& vhd) -> const std::string& { | ||
| // If the variant holds the SCSI LUN, query the guest for the device path. | ||
| if (std::holds_alternative<ULONG>(vhd)) | ||
| { | ||
| const auto lun = std::get<ULONG>(vhd); | ||
| auto it = m_attachedDisks.find(lun); | ||
| WI_ASSERT(it != m_attachedDisks.end() && it->second.Device.empty()); | ||
|
|
||
| auto kernelModulesPath = basePath / L"tools" / L"modules.vhd"; | ||
| it->second.Device = GetVhdDevicePath(lun); | ||
| vhd = it->second.Device; | ||
| } | ||
|
|
||
| #endif | ||
| return std::get<std::string>(vhd); | ||
| }; |
There was a problem hiding this comment.
The getDevicePath lambda modifies the vhd variant parameter in-place (line 409) and then returns a reference to the string inside the variant. This creates a subtle issue: if the variant is called multiple times on the same variant variable, it will fail the assertion on line 406 because Device will no longer be empty after the first call. Consider whether the variant should be modified in-place or if the function should return by value instead. The current design assumes each variant is only queried once, which may not be obvious to future maintainers.
| else | ||
| { | ||
| hcs::VirtualPMemController pmemController; | ||
| pmemController.MaximumCount = 0; | ||
| pmemController.MaximumSizeBytes = 0; | ||
| pmemController.Backing = hcs::VirtualPMemBackingType::Virtual; | ||
| auto attachPmemDisk = [&](PCWSTR path) { | ||
| ULONG deviceId = pmemController.MaximumCount; | ||
| pmemController.MaximumCount += 1; | ||
| hcs::VirtualPMemDevice vhd; | ||
| vhd.HostPath = path; | ||
| vhd.ReadOnly = true; | ||
| vhd.ImageFormat = hcs::VirtualPMemImageFormat::Vhd1; | ||
| pmemController.Devices[std::to_string(deviceId)] = std::move(vhd); | ||
| return std::format("/dev/pmem{}", deviceId); | ||
| }; | ||
|
|
||
| rootVhd = attachPmemDisk(m_settings.RootVhd.c_str()); | ||
| modulesVhd = attachPmemDisk(kernelModulesPath.c_str()); | ||
| vmSettings.Devices.VirtualPMem = std::move(pmemController); | ||
| } |
There was a problem hiding this comment.
The new PMEM VHD attachment feature (controlled by WslaFeatureFlagsPmemVhds) lacks test coverage. While other feature flags like WslaFeatureFlagsDnsTunneling, WslaFeatureFlagsVirtioFs, and WslaFeatureFlagsGPU have corresponding tests in test/windows/WSLATests.cpp, there are no tests for the PMEM code path. Consider adding tests that verify: 1) PMEM devices are correctly attached when the flag is enabled, 2) the device paths are correctly formatted as /dev/pmem{deviceId}, and 3) the VirtualPMemController schema is correctly serialized to JSON.
This PR introduces support for attaching VHDs (Virtual Hard Disks) as persistent memory (PMEM) devices in WSL, controlled by a new feature flag. It refactors how virtual disks are attached and mounted, adds new schema definitions for PMEM devices, and simplifies the mounting logic by integrating it directly into the VM creation JSON.