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

Draft
wants to merge 3 commits into
base: 4.19
Choose a base branch
from
Draft

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";

// 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
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