From edd537f8f53954ca269fe4f4512ec229825cace8 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 20 Jan 2025 16:13:37 +0530 Subject: [PATCH 1/2] kvm: fix disk controller for secure boot vm Fixes #9460 Signed-off-by: Abhishek Kumar --- .../hypervisor/kvm/resource/LibvirtComputingResource.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index f9d56f8301d5..923ee4ea407f 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -3149,11 +3149,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { 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); - } + disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2); } } From d10d51025cf151d6d8723e8a239c65ea7ca342d7 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 24 Apr 2025 13:33:51 +0530 Subject: [PATCH 2/2] kvm: allow skip forcing disk controller for Win VM with secure boot A VM or template detail can be added with key `win.skip.force.disk.controller` and value `true` to allow skipping forcing DATA disk controller for the VM. Signed-off-by: Abhishek Kumar --- .../java/com/cloud/vm/VmDetailConstants.java | 3 + .../resource/LibvirtComputingResource.java | 48 +++++++++-- .../LibvirtComputingResourceTest.java | 84 ++++++++++++++++++- 3 files changed, 122 insertions(+), 13 deletions(-) diff --git a/api/src/main/java/com/cloud/vm/VmDetailConstants.java b/api/src/main/java/com/cloud/vm/VmDetailConstants.java index f7d87ab18f1e..f237e04d8e16 100644 --- a/api/src/main/java/com/cloud/vm/VmDetailConstants.java +++ b/api/src/main/java/com/cloud/vm/VmDetailConstants.java @@ -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"; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 923ee4ea407f..39487101648b 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -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; @@ -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; @@ -2995,6 +2994,42 @@ public static DiskDef.DiskType getDiskType(KVMPhysicalDisk physicalDisk) { 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 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 details = vmSpec.getDetails(); final List disks = Arrays.asList(vmSpec.getDisks()); @@ -3146,14 +3181,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { disk.setDiscard(DiscardType.UNMAP); } } else { - if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) { - disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2); - } else { - disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2); - } - + defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot, + physicalDisk,devId, diskBusType, diskBusTypeData, details); } - } if (data instanceof VolumeObjectTO) { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index bbd1f8a73f2d..bee0d337a87b 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -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; @@ -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 { @@ -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 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()); @@ -6408,4 +6420,68 @@ public void createLinstorVdb() throws LibvirtException, InternalErrorException, assertEquals(DiskDef.DiscardType.UNMAP, rootDisk.getDiscard()); } } + + @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); + } }