Skip to content

kvm: fix disk controller for secure boot vm #10213

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

Closed
wants to merge 3 commits into from
Closed

kvm: fix disk controller for secure boot vm #10213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
edd537f
kvm: fix disk controller for secure boot vm
shwstppr Jan 20, 2025
d10d510
kvm: allow skip forcing disk controller for Win VM with secure boot
shwstppr Apr 24, 2025
9406e6e
Merge remote-tracking branch 'apache/4.19' into fix-kvm-securebootvm-…
shwstppr Apr 24, 2025
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
3 changes: 3 additions & 0 deletions api/src/main/java/com/cloud/vm/VmDetailConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ public interface VmDetailConstants {
String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number";
String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled";

// KVM specific, disk controllers
String KVM_WIN_SKIP_FORCE_DISK_CONTROLLER = "win.skip.force.disk.controller";
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, the root disk and data disk controller should not be changed when vm is stopped and started, in regardless of the vm settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weizhouapache In my test they are not really changed. Even at the deployment time, Windows VM with secure boot was always getting sata while other (Linux, etc) VMs were getting virtio


// Mac OSX guest specific (internal)
String SMC_PRESENT = "smc.present";
String FIRMWARE = "firmware";
Expand Down
52 changes: 39 additions & 13 deletions ...rvisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;


import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
import com.cloud.agent.api.HostVmStateReportEntry;
Expand Down Expand Up @@ -180,8 +179,8 @@
import com.cloud.network.Networks.RouterPrivateIpStrategy;
import com.cloud.network.Networks.TrafficType;
import com.cloud.resource.AgentStatusUpdater;
import com.cloud.resource.ResourceStatusUpdater;
import com.cloud.resource.RequestWrapper;
import com.cloud.resource.ResourceStatusUpdater;
import com.cloud.resource.ServerResource;
import com.cloud.resource.ServerResourceBase;
import com.cloud.storage.JavaStorageLayer;
Expand Down Expand Up @@ -3010,6 +3009,42 @@
return useBLOCKDiskType(physicalDisk) ? DiskDef.DiskType.BLOCK : DiskDef.DiskType.FILE;
}

/**
* Defines the disk configuration for the default pool type based on the provided parameters.
* It determines the appropriate disk settings depending on whether the disk is a data disk, whether
* it's a Windows template, whether UEFI is enabled, and whether secure boot is active.
*
* @param disk The disk definition object that will be configured with the disk settings.
* @param volume The volume (disk) object, containing information about the type of disk.
* @param isWindowsTemplate Flag indicating whether the template is a Windows template.
* @param isUefiEnabled Flag indicating whether UEFI is enabled.
* @param isSecureBoot Flag indicating whether secure boot is enabled.
* @param physicalDisk The physical disk object that contains the path to the disk.
* @param devId The device ID for the disk.
* @param diskBusType The disk bus type to use if not skipping force disk controller.
* @param diskBusTypeData The disk bus type to use for data disks, if applicable.
* @param details A map of VM details containing additional configuration values, such as whether to skip force
* disk controller.
*/
protected void defineDiskForDefaultPoolType(DiskDef disk, DiskTO volume, boolean isWindowsTemplate,
boolean isUefiEnabled, boolean isSecureBoot, KVMPhysicalDisk physicalDisk, int devId,
DiskDef.DiskBus diskBusType, DiskDef.DiskBus diskBusTypeData, Map<String, String> details) {
boolean skipForceDiskController = BooleanUtils.toBoolean(details.get(
VmDetailConstants.KVM_WIN_SKIP_FORCE_DISK_CONTROLLER));
boolean isDataDiskWithoutForceController = volume.getType() == Volume.Type.DATADISK &&
(!(isWindowsTemplate && isUefiEnabled) || skipForceDiskController);

if (isDataDiskWithoutForceController) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
if (isSecureBoot) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
} else {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
}
}
}

public void createVbd(final Connect conn, final VirtualMachineTO vmSpec, final String vmName, final LibvirtVMDef vm) throws InternalErrorException, LibvirtException, URISyntaxException {
final Map<String, String> details = vmSpec.getDetails();
final List<DiskTO> disks = Arrays.asList(vmSpec.getDisks());
Expand Down Expand Up @@ -3161,18 +3196,9 @@
disk.setDiscard(DiscardType.UNMAP);
}
} else {
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
if (isSecureBoot) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
} else {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
}
}

defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,

Check warning on line 3199 in plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

View check run for this annotation

Codecov / codecov/patch

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java#L3199 

Added line #L3199 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

@shwstppr If a user forces the disk type with this template flag, will Secure Boot still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavanaravapalli probably not for all guest OSes, but should work for newer ones, as #9460 reports VM works fine with virtio. Also, as this is optional, it is up to the operator to check and use

Copy link
Contributor

Choose a reason for hiding this comment

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

@shwstppr Thanks for the response. We should mention potential UEFI workflow issues in the flag description since guest OS support varies.

physicalDisk,devId, diskBusType, diskBusTypeData, details);
}

}

if (data instanceof VolumeObjectTO) {
Expand Down
84 changes: 80 additions & 4 deletions ...ors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@
import javax.xml.xpath.XPathExpressionException;
import javax.xml.xpath.XPathFactory;

import com.cloud.utils.net.NetUtils;

import com.cloud.vm.VmDetailConstants;
import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy;
import org.apache.cloudstack.storage.command.AttachAnswer;
import org.apache.cloudstack.storage.command.AttachCommand;
Expand Down Expand Up @@ -217,13 +214,15 @@
import com.cloud.template.VirtualMachineTemplate.BootloaderType;
import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.utils.script.Script;
import com.cloud.utils.net.NetUtils;
import com.cloud.utils.script.OutputInterpreter.OneLineParser;
import com.cloud.utils.script.Script;
import com.cloud.utils.ssh.SshHelper;
import com.cloud.vm.DiskProfile;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachine.PowerState;
import com.cloud.vm.VirtualMachine.Type;
import com.cloud.vm.VmDetailConstants;

@RunWith(MockitoJUnitRunner.class)
public class LibvirtComputingResourceTest {
Expand All @@ -240,6 +239,19 @@ public class LibvirtComputingResourceTest {
Connect connMock;
@Mock
LibvirtDomainXMLParser parserMock;
@Mock
private DiskDef diskDef;
@Mock
private DiskTO volume;
@Mock
private KVMPhysicalDisk physicalDisk;
@Mock
private Map<String, String> details;

private static final String PHYSICAL_DISK_PATH = "/path/to/disk";
private static final int DEV_ID = 1;
private static final DiskDef.DiskBus DISK_BUS_TYPE = DiskDef.DiskBus.VIRTIO;
private static final DiskDef.DiskBus DISK_BUS_TYPE_DATA = DiskDef.DiskBus.SCSI;

@Spy
private LibvirtComputingResource libvirtComputingResourceSpy = Mockito.spy(new LibvirtComputingResource());
Expand Down Expand Up @@ -6421,4 +6433,68 @@ public void testGetDiskModelFromVMDetailVirtioBlk() {
DiskDef.DiskBus diskBus = libvirtComputingResourceSpy.getDiskModelFromVMDetail(virtualMachineTO);
assertEquals(DiskDef.DiskBus.VIRTIOBLK, diskBus);
}

@Test
public void testDefineDiskForDefaultPoolType_DataDiskNonWinSkipForceController() {
when(volume.getType()).thenReturn(Volume.Type.DATADISK);
when(details.get(VmDetailConstants.KVM_WIN_SKIP_FORCE_DISK_CONTROLLER)).thenReturn("true");
when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
boolean isWindowsTemplate = false;
boolean isUefiEnabled = false;
boolean isSecureBoot = false;
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
verify(diskDef, times(1)).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
}

@Test
public void testDefineDiskForDefaultPoolType_DataDiskWinSecuredSkipForceController() {
when(volume.getType()).thenReturn(Volume.Type.DATADISK);
when(details.get(VmDetailConstants.KVM_WIN_SKIP_FORCE_DISK_CONTROLLER)).thenReturn("true");
when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
boolean isWindowsTemplate = true;
boolean isUefiEnabled = true;
boolean isSecureBoot = true;
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
verify(diskDef, times(1)).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
}

@Test
public void testDefineDiskForDefaultPoolType_WinSecureBootEnabledForced() {
when(volume.getType()).thenReturn(Volume.Type.DATADISK);
when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
boolean isWindowsTemplate = true;
boolean isUefiEnabled = true;
boolean isSecureBoot = true;
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
verify(diskDef, times(1)).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
}

@Test
public void testDefineDiskForDefaultPoolType_SecureBootDisabledForced() {
when(volume.getType()).thenReturn(Volume.Type.DATADISK);
when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
boolean isWindowsTemplate = true;
boolean isUefiEnabled = false;
boolean isSecureBoot = false;
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
verify(diskDef, times(1)).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
}

@Test
public void testDefineDiskForDefaultPoolType_NotDataDisk() {
when(volume.getType()).thenReturn(Volume.Type.ROOT);
when(details.get(VmDetailConstants.KVM_WIN_SKIP_FORCE_DISK_CONTROLLER)).thenReturn("false");
when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
boolean isWindowsTemplate = false;
boolean isUefiEnabled = false;
boolean isSecureBoot = false;
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
verify(diskDef, never()).defFileBasedDisk(PHYSICAL_DISK_PATH, PHYSICAL_DISK_PATH, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
verify(diskDef, times(1)).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE, DiskDef.DiskFmtType.QCOW2);
}
}
Loading