Skip to content

Commit e712409

Browse files
tjones60tjones60
committed
feedback and cleanup
1 parent 3890bfc commit e712409

File tree

6 files changed

+183
-176
lines changed

6 files changed

+183
-176
lines changed

‎petri/src/vm/hyperv/mod.rs‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,6 @@ impl PetriVmmBackend for HyperVPetriBackend {
174174
proc_topology,
175175
vmgs,
176176
tpm,
177-
ide_controllers,
178177
vmbus_storage_controllers,
179178
} = config;
180179

@@ -187,14 +186,16 @@ impl PetriVmmBackend for HyperVPetriBackend {
187186

188187
let temp_dir = tempfile::tempdir()?;
189188

190-
let (guest_state_isolation_type, generation, uefi_config, openhcl_config) = match firmware {
189+
let (guest_state_isolation_type, generation, uefi_config, openhcl_config) = match &firmware
190+
{
191191
Firmware::LinuxDirect { .. } | Firmware::OpenhclLinuxDirect { .. } => {
192192
todo!("linux direct not supported on hyper-v")
193193
}
194194
Firmware::Pcat {
195195
guest: _,
196196
bios_firmware: _, // TODO
197197
svga_firmware: _, // TODO
198+
ide_controllers: _,
198199
} => (
199200
powershell::HyperVGuestStateIsolationType::Disabled,
200201
powershell::HyperVGeneration::One,
@@ -348,7 +349,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
348349
}) = uefi_config
349350
{
350351
vm.set_secure_boot(
351-
secure_boot_enabled,
352+
*secure_boot_enabled,
352353
secure_boot_template.map(|t| match t {
353354
SecureBootTemplate::MicrosoftWindows => {
354355
HyperVSecureBootTemplate::MicrosoftWindows
@@ -361,7 +362,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
361362
.await?;
362363

363364
// TODO: Disable frontpage for non-OpenHCL Hyper-V VMs
364-
if disable_frontpage && properties.is_openhcl {
365+
if *disable_frontpage && properties.is_openhcl {
365366
append_cmdline(
366367
&mut openhcl_command_line,
367368
"OPENHCL_DISABLE_UEFI_FRONTPAGE=1",
@@ -373,12 +374,12 @@ impl PetriVmmBackend for HyperVPetriBackend {
373374
&mut openhcl_command_line,
374375
format!(
375376
"HCL_DEFAULT_BOOT_ALWAYS_ATTEMPT={}",
376-
if default_boot_always_attempt { 1 } else { 0 }
377+
if *default_boot_always_attempt { 1 } else { 0 }
377378
),
378379
);
379380
};
380381

381-
if enable_vpci_boot {
382+
if *enable_vpci_boot {
382383
todo!("hyperv nvme boot");
383384
}
384385
}
@@ -401,7 +402,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
401402
vm.set_imc(&imc_hive).await?;
402403
}
403404

404-
let vtl2_settings = if let Some((
405+
if let Some((
405406
src_igvm_file,
406407
OpenHclConfig {
407408
vtl2_nvme_boot: _, // TODO, see #1649.
@@ -440,7 +441,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
440441
vm.set_vm_firmware_command_line(openhcl_command_line.as_ref().unwrap())
441442
.await?;
442443

443-
vm.set_vmbus_redirect(vmbus_redirect).await?;
444+
vm.set_vmbus_redirect(*vmbus_redirect).await?;
444445

445446
// Attempt to enable COM3 and use that to get KMSG logs, otherwise
446447
// fall back to use diag_client.
@@ -497,10 +498,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
497498
if let Some(settings) = &vtl2_settings {
498499
vm.set_base_vtl2_settings(settings).await?;
499500
}
500-
vtl2_settings
501-
} else {
502-
None
503-
};
501+
}
504502

505503
let serial_pipe_path = vm.set_vm_com_port(1).await?;
506504
let serial_log_file = log_source.log_file("guest")?;
@@ -510,7 +508,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
510508
));
511509

512510
// Add IDE storage
513-
if let Some(ide_controllers) = &ide_controllers {
511+
if let Some(ide_controllers) = firmware.ide_controllers() {
514512
for (controller_number, controller) in ide_controllers.iter().enumerate() {
515513
for (controller_location, disk) in controller.iter().enumerate() {
516514
if let Some(disk) = disk {
@@ -590,11 +588,7 @@ impl PetriVmmBackend for HyperVPetriBackend {
590588
driver: driver.clone(),
591589
properties,
592590
},
593-
PetriVmRuntimeConfig {
594-
vtl2_settings,
595-
ide_controllers,
596-
vmbus_storage_controllers,
597-
},
591+
firmware.into_runtime_config(vmbus_storage_controllers),
598592
))
599593
}
600594
}

‎petri/src/vm/mod.rs‎

Lines changed: 63 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,6 @@ pub struct PetriVmConfig {
148148
pub vmgs: PetriVmgsResource,
149149
/// TPM configuration
150150
pub tpm: Option<TpmConfig>,
151-
/// IDE controllers and associated disks
152-
pub ide_controllers: Option<[[Option<Drive>; 2]; 2]>,
153151
/// Storage controllers and associated disks
154152
pub vmbus_storage_controllers: HashMap<Guid, VmbusStorageController>,
155153
}
@@ -295,9 +293,6 @@ impl<T: PetriVmmBackend> PetriVmBuilder<T> {
295293
Firmware::Uefi { .. } | Firmware::OpenhclUefi { .. } => BootDeviceType::Scsi,
296294
};
297295

298-
let ide_controllers = matches!(artifacts.firmware, Firmware::Pcat { .. })
299-
.then(|| [[None, None], [None, None]]);
300-
301296
Ok(Self {
302297
backend: artifacts.backend,
303298
config: PetriVmConfig {
@@ -310,7 +305,6 @@ impl<T: PetriVmmBackend> PetriVmBuilder<T> {
310305

311306
vmgs: PetriVmgsResource::Ephemeral,
312307
tpm: None,
313-
ide_controllers,
314308
vmbus_storage_controllers: HashMap::new(),
315309
},
316310
modify_vmm_config: None,
@@ -413,6 +407,7 @@ impl<T: PetriVmmBackend> PetriVmBuilder<T> {
413407
fn add_agent_disk_inner(self, target_vtl: Vtl) -> Self {
414408
let (agent_image, controller_id) = match target_vtl {
415409
Vtl::Vtl0 => (self.agent_image.as_ref(), PETRI_SCSI_VTL0_CONTROLLER),
410+
Vtl::Vtl1 => panic!("no VTL1 agent disk"),
416411
Vtl::Vtl2 => (
417412
self.openhcl_agent_image.as_ref(),
418413
PETRI_SCSI_VTL2_CONTROLLER,
@@ -1092,14 +1087,17 @@ impl<T: PetriVmmBackend> PetriVmBuilder<T> {
10921087
target_vtl: Vtl,
10931088
controller_type: VmbusStorageType,
10941089
) -> Self {
1095-
if self.config.vmbus_storage_controllers.contains_key(id) {
1096-
panic!("storage controller {id} already exists");
1090+
if self
1091+
.config
1092+
.vmbus_storage_controllers
1093+
.insert(
1094+
*id,
1095+
VmbusStorageController::new(target_vtl, controller_type),
1096+
)
1097+
.is_some()
1098+
{
1099+
panic!("storage controller {id} already existed");
10971100
}
1098-
1099-
self.config.vmbus_storage_controllers.insert(
1100-
*id,
1101-
VmbusStorageController::new(target_vtl, controller_type),
1102-
);
11031101
self
11041102
}
11051103

@@ -1129,8 +1127,8 @@ impl<T: PetriVmmBackend> PetriVmBuilder<T> {
11291127
controller_location: u8,
11301128
) -> Self {
11311129
self.config
1132-
.ide_controllers
1133-
.as_mut()
1130+
.firmware
1131+
.ide_controllers_mut()
11341132
.expect("Host IDE requires PCAT with no HCL")[controller_number as usize]
11351133
[controller_location as usize] = Some(drive);
11361134

@@ -1865,6 +1863,8 @@ pub enum Firmware {
18651863
bios_firmware: ResolvedOptionalArtifact,
18661864
/// The SVGA firmware to use.
18671865
svga_firmware: ResolvedOptionalArtifact,
1866+
/// IDE controllers and associated disks
1867+
ide_controllers: [[Option<Drive>; 2]; 2],
18681868
},
18691869
/// Boot a PCAT-based VM with OpenHCL in VTL2.
18701870
OpenhclPcat {
@@ -1987,6 +1987,7 @@ impl Firmware {
19871987
guest,
19881988
bios_firmware: resolver.try_require(PCAT_FIRMWARE_X64).erase(),
19891989
svga_firmware: resolver.try_require(SVGA_FIRMWARE_X64).erase(),
1990+
ide_controllers: [[None, None], [None, None]],
19901991
}
19911992
}
19921993

@@ -2174,12 +2175,30 @@ impl Firmware {
21742175
}
21752176
}
21762177

2177-
fn into_openhcl_config(self) -> Option<OpenHclConfig> {
2178+
fn into_runtime_config(
2179+
self,
2180+
vmbus_storage_controllers: HashMap<Guid, VmbusStorageController>,
2181+
) -> PetriVmRuntimeConfig {
21782182
match self {
21792183
Firmware::OpenhclLinuxDirect { openhcl_config, .. }
21802184
| Firmware::OpenhclUefi { openhcl_config, .. }
2181-
| Firmware::OpenhclPcat { openhcl_config, .. } => Some(openhcl_config),
2182-
Firmware::LinuxDirect { .. } | Firmware::Pcat { .. } | Firmware::Uefi { .. } => None,
2185+
| Firmware::OpenhclPcat { openhcl_config, .. } => PetriVmRuntimeConfig {
2186+
vtl2_settings: openhcl_config.vtl2_settings,
2187+
ide_controllers: None,
2188+
vmbus_storage_controllers,
2189+
},
2190+
Firmware::Pcat {
2191+
ide_controllers, ..
2192+
} => PetriVmRuntimeConfig {
2193+
vtl2_settings: None,
2194+
ide_controllers: Some(ide_controllers),
2195+
vmbus_storage_controllers,
2196+
},
2197+
Firmware::LinuxDirect { .. } | Firmware::Uefi { .. } => PetriVmRuntimeConfig {
2198+
vtl2_settings: None,
2199+
ide_controllers: None,
2200+
vmbus_storage_controllers,
2201+
},
21832202
}
21842203
}
21852204

@@ -2229,6 +2248,24 @@ impl Firmware {
22292248
self.openhcl_config_mut()
22302249
.map(|c| c.vtl2_settings.get_or_insert_with(default_vtl2_settings))
22312250
}
2251+
2252+
fn ide_controllers(&self) -> Option<&[[Option<Drive>; 2]; 2]> {
2253+
match self {
2254+
Firmware::Pcat {
2255+
ide_controllers, ..
2256+
} => Some(ide_controllers),
2257+
_ => None,
2258+
}
2259+
}
2260+
2261+
fn ide_controllers_mut(&mut self) -> Option<&mut [[Option<Drive>; 2]; 2]> {
2262+
match self {
2263+
Firmware::Pcat {
2264+
ide_controllers, ..
2265+
} => Some(ide_controllers),
2266+
_ => None,
2267+
}
2268+
}
22322269
}
22332270

22342271
/// The guest the VM will boot into. A boot drive with the chosen setup
@@ -2575,6 +2612,8 @@ fn default_vtl2_settings() -> Vtl2Settings {
25752612
pub enum Vtl {
25762613
/// VTL 0
25772614
Vtl0 = 0,
2615+
/// VTL 1
2616+
Vtl1 = 1,
25782617
/// VTL 2
25792618
Vtl2 = 2,
25802619
}
@@ -2632,27 +2671,20 @@ impl VmbusStorageController {
26322671
drive: Drive,
26332672
allow_modify_existing: bool,
26342673
) -> u32 {
2635-
let lun = if let Some(lun) = lun {
2636-
if !allow_modify_existing && self.drives.contains_key(&lun) {
2637-
panic!("a disk with lun {lun} already exists on this controller");
2638-
}
2639-
lun
2640-
} else {
2674+
let lun = lun.unwrap_or_else(|| {
26412675
// find the first available lun
26422676
let mut lun = None;
26432677
for x in 0..u8::MAX as u32 {
26442678
if !self.drives.contains_key(&x) {
26452679
lun = Some(x)
26462680
}
26472681
}
2648-
if let Some(lun) = lun {
2649-
lun
2650-
} else {
2651-
panic!("all locations on this controller are in use")
2652-
}
2653-
};
2682+
lun.expect("all locations on this controller are in use")
2683+
});
26542684

2655-
self.drives.insert(lun, drive);
2685+
if self.drives.insert(lun, drive).is_some() && !allow_modify_existing {
2686+
panic!("a disk with lun {lun} already existed on this controller");
2687+
}
26562688

26572689
lun
26582690
}

‎petri/src/vm/openvmm/construct.rs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ impl PetriVmConfigOpenVmm {
113113
proc_topology,
114114
vmgs,
115115
tpm: tpm_config,
116-
ide_controllers,
117116
vmbus_storage_controllers,
118117
} = petri_vm_config;
119118

@@ -174,7 +173,7 @@ impl PetriVmConfigOpenVmm {
174173
let mut vmbus_devices = Vec::new();
175174

176175
// Add IDE storage
177-
if let Some(ide_controllers) = &ide_controllers {
176+
if let Some(ide_controllers) = firmware.ide_controllers() {
178177
for (controller_number, controller) in ide_controllers.iter().enumerate() {
179178
for (controller_location, drive) in controller.iter().enumerate() {
180179
if let Some(drive) = drive {
@@ -237,6 +236,7 @@ impl PetriVmConfigOpenVmm {
237236
vmbus_devices.push((
238237
match controller.target_vtl {
239238
crate::Vtl::Vtl0 => DeviceVtl::Vtl0,
239+
crate::Vtl::Vtl1 => DeviceVtl::Vtl1,
240240
crate::Vtl::Vtl2 => DeviceVtl::Vtl2,
241241
},
242242
ScsiControllerHandle {
@@ -578,14 +578,8 @@ impl PetriVmConfigOpenVmm {
578578
None
579579
};
580580

581-
let vtl2_settings = firmware.into_openhcl_config().and_then(|c| c.vtl2_settings);
582-
583581
Ok(Self {
584-
runtime_config: crate::PetriVmRuntimeConfig {
585-
vtl2_settings,
586-
ide_controllers,
587-
vmbus_storage_controllers,
588-
},
582+
runtime_config: firmware.into_runtime_config(vmbus_storage_controllers),
589583
arch,
590584
host_log_levels,
591585
config,
@@ -741,6 +735,7 @@ impl PetriVmConfigSetupCore<'_> {
741735
bios_firmware: firmware,
742736
guest: _, // load_boot_disk
743737
svga_firmware: _, // config_video
738+
ide_controllers: _,
744739
},
745740
) => {
746741
let firmware = openvmm_pcat_locator::find_pcat_bios(firmware.get())

‎vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -558,22 +558,20 @@ async fn create_keepalive_test_config(
558558
})
559559
})
560560
// Assign the fault controller to VTL2
561-
.with_custom_vtl2_settings(move |v| {
562-
v.dynamic.as_mut().unwrap().storage_controllers.push(
563-
Vtl2StorageControllerBuilder::new(ControllerType::Scsi)
564-
.with_instance_id(scsi_instance)
565-
.add_lun(
566-
Vtl2LunBuilder::disk()
567-
.with_location(vtl0_nvme_lun)
568-
.with_physical_device(Vtl2StorageBackingDeviceBuilder::new(
569-
ControllerType::Nvme,
570-
NVME_INSTANCE,
571-
KEEPALIVE_VTL2_NSID,
572-
)),
573-
)
574-
.build(),
575-
);
576-
})
561+
.add_vtl2_storage_controller(
562+
Vtl2StorageControllerBuilder::new(ControllerType::Scsi)
563+
.with_instance_id(scsi_instance)
564+
.add_lun(
565+
Vtl2LunBuilder::disk()
566+
.with_location(vtl0_nvme_lun)
567+
.with_physical_device(Vtl2StorageBackingDeviceBuilder::new(
568+
ControllerType::Nvme,
569+
NVME_INSTANCE,
570+
KEEPALIVE_VTL2_NSID,
571+
)),
572+
)
573+
.build(),
574+
)
577575
.run()
578576
.await
579577
}

0 commit comments

Comments
 (0)