You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/07/01 05:27:48 UTC

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5149: Refactor and improvements for method com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.createVMFromSpec()

GabrielBrascher commented on a change in pull request #5149:
URL: https://github.com/apache/cloudstack/pull/5149#discussion_r661963262



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -209,6 +209,67 @@
 public class LibvirtComputingResource extends ServerResourceBase implements ServerResource, VirtualRouterDeployer {
     private static final Logger s_logger = Logger.getLogger(LibvirtComputingResource.class);
 
+    private static final String LEGACY = "legacy";
+    private static final String SECURE = "secure";
+
+    /**
+     * Machine type.
+     */
+    private static final String PC = "pc";
+    private static final String VIRT = "virt";
+
+    /**
+     * Possible devices to add to VM.
+     */
+    private static final String TABLET = "tablet";
+    private static final String USB = "usb";
+    private static final String MOUSE = "mouse";
+    private static final String KEYBOARD = "keyboard";
+
+    /**
+     * Policies used by VM.
+     */
+    private static final String RESTART = "restart";
+    private static final String DESTROY = "destroy";
+
+    private static final String KVMCLOCK = "kvmclock";
+    private static final String HYPERVCLOCK = "hypervclock";
+    private static final String WINDOWS = "Windows";
+    private static final String Q35 = "q35";
+    private static final String PTY = "pty";
+    private static final String VNC = "vnc";
+
+    /**
+     * Acronym of System Management Mode. Perform low-level system management operations while an OS is running.
+     */
+    private static final String SMM = "smm";
+    /**
+     * Acronym of Advanced Configuration and Power Interface.<br>
+     * Provides an open standard that operating systems can use to discover and configure
+     * computer hardware components, to perform power management.
+     */
+    private static final String ACPI = "acpi";
+    /**
+     * Acronym of Advanced Programmable Interrupt Controllers.<br>
+     * With an I/O APIC, operating systems can use more than 16 interrupt requests (IRQs)
+     * and therefore avoid IRQ sharing for improved reliability.
+     */
+    private static final String APIC = "apic";
+    /**
+     * Acronym of Physical Address Extension. Feature implemented in modern x86 processors.<br>
+     * PAE extends memory addressing capabilities, allowing more than 4 GB of random access memory (RAM) to be used.
+     */
+    private static final String PAE = "pae";
+    /**
+     * Libvirt only suport guest cpu mode in versions from 0.9.10.
+     */
+    private static final int MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_MODE = 9010;
+    private static final int MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE = 9000;
+    /**
+     * Verify if guest is ARM64

Review comment:
       Verify if guest is ARM64
   :arrow_down: 
   1. Verifies if the guest is ARM64
   OR
   2. Constant that defines ARM64 (aarch64) guest architectures.

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+            addExtraConfigComponent(extraConfig, vm);
         }
+    }
 
-            vm.addComp(guest);
+    /**
+     * Add devices components to VM.
+     */
+    protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef guest, int vcpus, boolean isUefiEnabled) {
+        DevicesDef devices = new DevicesDef();
+        devices.setEmulatorPath(_hypervisorPath);
+        devices.setGuestType(guest.getGuestType());
+        devices.addDevice(createSerialDef());
 
-        final GuestResourceDef grd = new GuestResourceDef();
+        if (_rngEnable) {
+            devices.addDevice(createRngDef());
+        }
 
-        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
-            grd.setMemBalloning(true);
-            grd.setCurrentMem(vmTO.getMinRam() / 1024);
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
-        } else {
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        devices.addDevice(createChannelDef(vmTO));
+        devices.addDevice(createWatchDogDef());
+        devices.addDevice(createVideoDef());
+        devices.addDevice(createConsoleDef());
+        devices.addDevice(createGraphicDef(vmTO));
+        devices.addDevice(createTabletInputDef());
+
+        if (isGuestAarch64()) {
+            createArm64UsbDef(devices);
         }
-        final int vcpus = vmTO.getCpus();
-        grd.setVcpuNum(vcpus);
-        vm.addComp(grd);
 
-        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
-            final CpuModeDef cmd = new CpuModeDef();
-            cmd.setMode(_guestCpuMode);
-            cmd.setModel(_guestCpuModel);
-            if (vmTO.getType() == VirtualMachine.Type.User) {
-                cmd.setFeatures(_cpuFeatures);
-            }
-            setCpuTopology(cmd, vcpus, vmTO.getDetails());
-            vm.addComp(cmd);
+        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+        if (busT == null) {
+            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
+        }
+
+        if (busT == DiskDef.DiskBus.SCSI) {
+            devices.addDevice(createSCSIDef(vcpus));
         }
+        return devices;
+    }
 
-        if (_hypervisorLibvirtVersion >= 9000) {
-            final CpuTuneDef ctd = new CpuTuneDef();
-            /**
-             A 4.0.X/4.1.X management server doesn't send the correct JSON
-             command for getMinSpeed, it only sends a 'speed' field.
+    protected WatchDogDef createWatchDogDef() {
+        return new WatchDogDef(_watchDogAction, _watchDogModel);
+    }
 
-             So if getMinSpeed() returns null we fall back to getSpeed().
+    /*
+     * Add an explicit USB devices for ARM64
+     */
+    protected void createArm64UsbDef(DevicesDef devices) {
+        devices.addDevice(new InputDef(KEYBOARD, USB));
+        devices.addDevice(new InputDef(MOUSE, USB));
+        devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    }
 
-             This way a >4.1 agent can work communicate a <=4.1 management server
+    protected InputDef createTabletInputDef() {
+        return new InputDef(TABLET, USB);
+    }
 
-             This change is due to the overcommit feature in 4.2
-             */
-            if (vmTO.getMinSpeed() != null) {
-                ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
-            } else {
-                ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
-            }
+    /**
+     * Add the VNC port passwd here, get the passwd from the vmInstance.
+     */
+    protected GraphicDef createGraphicDef(VirtualMachineTO vmTO) {
+        return new GraphicDef(VNC, (short)0, true, vmTO.getVncAddr(), vmTO.getVncPassword(), null);
+    }
 
-            setQuotaAndPeriod(vmTO, ctd);
+    /**
+     * Add a VirtIO channel for the Qemu Guest Agent tools.
+     */
+    protected ChannelDef createChannelDef(VirtualMachineTO vmTO) {
+        File virtIoChannel = Paths.get(_qemuSocketsPath.getPath(), vmTO.getName() + "." + _qemuGuestAgentSocketName).toFile();
+        return new ChannelDef(_qemuGuestAgentSocketName, ChannelDef.ChannelType.UNIX, virtIoChannel);
+    }
 
-            vm.addComp(ctd);
-        }
+    /**
+     * If we're using virtio scsi, then we need to add a virtual scsi controller.
+     */
+    protected SCSIDef createSCSIDef(int vcpus) {
+        return new SCSIDef((short)0, 0, 0, 9, 0, vcpus);
+    }
 
-        final FeaturesDef features = new FeaturesDef();
-        features.addFeatures("pae");
-        features.addFeatures("apic");
-        features.addFeatures("acpi");
-        if (isUefiEnabled && isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-            features.addFeatures("smm");
-        }
+    protected ConsoleDef createConsoleDef() {
+        return new ConsoleDef(PTY, null, null, (short)0);
+    }
 
-        //KVM hyperv enlightenment features based on OS Type
-        enlightenWindowsVm(vmTO, features);
+    protected VideoDef createVideoDef() {
+        return new VideoDef(_videoHw, _videoRam);
+    }
 
-        vm.addComp(features);
+    protected RngDef createRngDef() {
+        return new RngDef(_rngPath, _rngBackendModel, _rngRateBytes, _rngRatePeriod);
+    }
 
-        final TermPolicy term = new TermPolicy();
-        term.setCrashPolicy("destroy");
-        term.setPowerOffPolicy("destroy");
-        term.setRebootPolicy("restart");
-        vm.addComp(term);
+    protected SerialDef createSerialDef() {
+        return new SerialDef(PTY, null, (short)0);
+    }
 
-        final ClockDef clock = new ClockDef();
-        if (vmTO.getOs().startsWith("Windows")) {
+    protected ClockDef createClockDef(final VirtualMachineTO vmTO) {
+        ClockDef clock = new ClockDef();
+        if (org.apache.commons.lang.StringUtils.startsWith(vmTO.getOs(), WINDOWS)) {
             clock.setClockOffset(ClockDef.ClockOffset.LOCALTIME);
-            clock.setTimer("hypervclock", null, null);
-        } else if (vmTO.getType() != VirtualMachine.Type.User || isGuestPVEnabled(vmTO.getOs())) {
-            if (_hypervisorLibvirtVersion >= 9 * 1000 + 10) {
-                clock.setTimer("kvmclock", null, null, _noKvmClock);
-            }
+            clock.setTimer(HYPERVCLOCK, null, null);
+        } else if ((vmTO.getType() != VirtualMachine.Type.User || isGuestPVEnabled(vmTO.getOs())) && _hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_MODE) {
+            clock.setTimer(KVMCLOCK, null, null, _noKvmClock);
         }
+        return clock;
+    }
 
-        vm.addComp(clock);
+    protected TermPolicy createTermPolicy() {
+        TermPolicy term = new TermPolicy();
+        term.setCrashPolicy(DESTROY);
+        term.setPowerOffPolicy(DESTROY);
+        term.setRebootPolicy(RESTART);
+        return term;
+    }
 
-        final DevicesDef devices = new DevicesDef();
-        devices.setEmulatorPath(_hypervisorPath);
-        devices.setGuestType(guest.getGuestType());
+    protected FeaturesDef createFeaturesDef(Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot) {
+        FeaturesDef features = new FeaturesDef();
+        features.addFeatures(PAE);
+        features.addFeatures(APIC);
+        features.addFeatures(ACPI);
+        if (isUefiEnabled && isSecureBoot) {
+            features.addFeatures(SMM);
+        }
+        return features;
+    }
 
-        final SerialDef serial = new SerialDef("pty", null, (short)0);
-        devices.addDevice(serial);
+    /**
+     * A 4.0.X/4.1.X management server doesn't send the correct JSON
+     * command for getMinSpeed, it only sends a 'speed' field.<br>
+     * So, to create a cpu tune,  if getMinSpeed() returns null we fall back to getSpeed().<br>
+     * This way a >4.1 agent can work communicate a <=4.1 management server.<br>
+     * This change is due to the overcommit feature in 4.2.
+     */
+    protected CpuTuneDef createCpuTuneDef(VirtualMachineTO vmTO) {
+        CpuTuneDef ctd = new CpuTuneDef();
+        int shares = vmTO.getCpus() * (vmTO.getMinSpeed() != null ? vmTO.getMinSpeed() : vmTO.getSpeed());
+        ctd.setShares(shares);
+        setQuotaAndPeriod(vmTO, ctd);
+        return ctd;
+    }
 
-        if (_rngEnable) {
-            final RngDef rngDevice = new RngDef(_rngPath, _rngBackendModel, _rngRateBytes, _rngRatePeriod);
-            devices.addDevice(rngDevice);
+    private CpuModeDef createCpuModeDef(VirtualMachineTO vmTO, int vcpus) {
+        final CpuModeDef cmd = new CpuModeDef();
+        cmd.setMode(_guestCpuMode);
+        cmd.setModel(_guestCpuModel);
+        if (VirtualMachine.Type.User.equals(vmTO.getType())) {
+            cmd.setFeatures(_cpuFeatures);
         }
+        setCpuTopology(cmd, vcpus, vmTO.getDetails());
+        return cmd;
+    }
 
-        /* Add a VirtIO channel for the Qemu Guest Agent tools */
-        File virtIoChannel = Paths.get(_qemuSocketsPath.getPath(), vmTO.getName() + "." + _qemuGuestAgentSocketName).toFile();
-        devices.addDevice(new ChannelDef(_qemuGuestAgentSocketName, ChannelDef.ChannelType.UNIX, virtIoChannel));
-
-        devices.addDevice(new WatchDogDef(_watchDogAction, _watchDogModel));
+    /**
+     * Creates guest resources based in VM specification.
+     */
+    protected GuestResourceDef createGuestResourceDef(VirtualMachineTO vmTO) {
+        GuestResourceDef grd = new GuestResourceDef();
 
-        final VideoDef videoCard = new VideoDef(_videoHw, _videoRam);
-        devices.addDevice(videoCard);
+        grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
+            grd.setMemBalloning(true);
+            grd.setCurrentMem(vmTO.getMinRam() / 1024);
+        }
+        grd.setVcpuNum(vmTO.getCpus());
+        return grd;
+    }
 
-        final ConsoleDef console = new ConsoleDef("pty", null, null, (short)0);
-        devices.addDevice(console);
+    private void configureGuestIfUefiEnabled(boolean isSecureBoot, String bootMode, GuestDef guest) {
+        setGuestLoader(bootMode, SECURE, guest, GuestDef.GUEST_LOADER_SECURE);
+        setGuestLoader(bootMode, LEGACY, guest, GuestDef.GUEST_LOADER_LEGACY);
 
-        //add the VNC port passwd here, get the passwd from the vmInstance.
-        final String passwd = vmTO.getVncPassword();
-        final GraphicDef grap = new GraphicDef("vnc", (short)0, true, vmTO.getVncAddr(), passwd, null);
-        devices.addDevice(grap);
+        if (isUefiPropertieNotNull(GuestDef.GUEST_NVRAM_PATH)) {
+            guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
+        }
 
-        final InputDef input = new InputDef("tablet", "usb");
-        devices.addDevice(input);
+        if (isSecureBoot && isUefiPropertieNotNull(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) && SECURE.equalsIgnoreCase(bootMode)) {
+            guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
+        } else if (isUefiPropertieNotNull(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY)) {
+            guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
+        }
+    }
 
-        // Add an explicit USB devices for ARM64
-        if (_guestCpuArch != null && _guestCpuArch.equals("aarch64")) {
-            devices.addDevice(new InputDef("keyboard", "usb"));
-            devices.addDevice(new InputDef("mouse", "usb"));
-            devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    private void setGuestLoader(String bootMode, String mode, GuestDef guest, String propertie) {
+        if (isUefiPropertieNotNull(propertie) && mode.equalsIgnoreCase(bootMode)) {
+            guest.setLoader(_uefiProperties.getProperty(propertie));
         }
+    }
 
-        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+    private boolean isUefiPropertieNotNull(String propertie) {
+        return _uefiProperties.getProperty(propertie) != null;
+    }
 
-        if (busT == null) {
-            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
-        }
+    private boolean isGuestAarch64() {
+        return AARCH64.equals(_guestCpuArch);
+    }
 
-        // If we're using virtio scsi, then we need to add a virtual scsi controller
-        if (busT == DiskDef.DiskBus.SCSI) {
-            final SCSIDef sd = new SCSIDef((short)0, 0, 0, 9, 0, vcpus);
-            devices.addDevice(sd);
-        }
+    /**
+     * Create a guest definition from a VM specification.

Review comment:
       Some documentations have the verb as Imperative (e.g. **create**), some Indicative (e.g. it **creates**).
   This is a small observation, but maybe you can normalize them. (I would go to the indicative form; but feel free to choose)

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+            addExtraConfigComponent(extraConfig, vm);
         }
+    }
 
-            vm.addComp(guest);
+    /**
+     * Add devices components to VM.
+     */
+    protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef guest, int vcpus, boolean isUefiEnabled) {
+        DevicesDef devices = new DevicesDef();
+        devices.setEmulatorPath(_hypervisorPath);
+        devices.setGuestType(guest.getGuestType());
+        devices.addDevice(createSerialDef());
 
-        final GuestResourceDef grd = new GuestResourceDef();
+        if (_rngEnable) {
+            devices.addDevice(createRngDef());
+        }
 
-        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
-            grd.setMemBalloning(true);
-            grd.setCurrentMem(vmTO.getMinRam() / 1024);
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
-        } else {
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        devices.addDevice(createChannelDef(vmTO));
+        devices.addDevice(createWatchDogDef());
+        devices.addDevice(createVideoDef());
+        devices.addDevice(createConsoleDef());
+        devices.addDevice(createGraphicDef(vmTO));
+        devices.addDevice(createTabletInputDef());
+
+        if (isGuestAarch64()) {
+            createArm64UsbDef(devices);
         }
-        final int vcpus = vmTO.getCpus();
-        grd.setVcpuNum(vcpus);
-        vm.addComp(grd);
 
-        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
-            final CpuModeDef cmd = new CpuModeDef();
-            cmd.setMode(_guestCpuMode);
-            cmd.setModel(_guestCpuModel);
-            if (vmTO.getType() == VirtualMachine.Type.User) {
-                cmd.setFeatures(_cpuFeatures);
-            }
-            setCpuTopology(cmd, vcpus, vmTO.getDetails());
-            vm.addComp(cmd);
+        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+        if (busT == null) {
+            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
+        }
+
+        if (busT == DiskDef.DiskBus.SCSI) {
+            devices.addDevice(createSCSIDef(vcpus));
         }
+        return devices;
+    }
 
-        if (_hypervisorLibvirtVersion >= 9000) {
-            final CpuTuneDef ctd = new CpuTuneDef();
-            /**
-             A 4.0.X/4.1.X management server doesn't send the correct JSON
-             command for getMinSpeed, it only sends a 'speed' field.
+    protected WatchDogDef createWatchDogDef() {
+        return new WatchDogDef(_watchDogAction, _watchDogModel);
+    }
 
-             So if getMinSpeed() returns null we fall back to getSpeed().
+    /*
+     * Add an explicit USB devices for ARM64
+     */
+    protected void createArm64UsbDef(DevicesDef devices) {
+        devices.addDevice(new InputDef(KEYBOARD, USB));
+        devices.addDevice(new InputDef(MOUSE, USB));
+        devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    }
 
-             This way a >4.1 agent can work communicate a <=4.1 management server
+    protected InputDef createTabletInputDef() {
+        return new InputDef(TABLET, USB);
+    }
 
-             This change is due to the overcommit feature in 4.2
-             */
-            if (vmTO.getMinSpeed() != null) {
-                ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
-            } else {
-                ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
-            }
+    /**
+     * Add the VNC port passwd here, get the passwd from the vmInstance.
+     */
+    protected GraphicDef createGraphicDef(VirtualMachineTO vmTO) {
+        return new GraphicDef(VNC, (short)0, true, vmTO.getVncAddr(), vmTO.getVncPassword(), null);
+    }
 
-            setQuotaAndPeriod(vmTO, ctd);
+    /**
+     * Add a VirtIO channel for the Qemu Guest Agent tools.
+     */
+    protected ChannelDef createChannelDef(VirtualMachineTO vmTO) {
+        File virtIoChannel = Paths.get(_qemuSocketsPath.getPath(), vmTO.getName() + "." + _qemuGuestAgentSocketName).toFile();
+        return new ChannelDef(_qemuGuestAgentSocketName, ChannelDef.ChannelType.UNIX, virtIoChannel);
+    }
 
-            vm.addComp(ctd);
-        }
+    /**
+     * If we're using virtio scsi, then we need to add a virtual scsi controller.
+     */
+    protected SCSIDef createSCSIDef(int vcpus) {
+        return new SCSIDef((short)0, 0, 0, 9, 0, vcpus);
+    }
 
-        final FeaturesDef features = new FeaturesDef();
-        features.addFeatures("pae");
-        features.addFeatures("apic");
-        features.addFeatures("acpi");
-        if (isUefiEnabled && isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-            features.addFeatures("smm");
-        }
+    protected ConsoleDef createConsoleDef() {
+        return new ConsoleDef(PTY, null, null, (short)0);
+    }
 
-        //KVM hyperv enlightenment features based on OS Type
-        enlightenWindowsVm(vmTO, features);
+    protected VideoDef createVideoDef() {
+        return new VideoDef(_videoHw, _videoRam);
+    }
 
-        vm.addComp(features);
+    protected RngDef createRngDef() {
+        return new RngDef(_rngPath, _rngBackendModel, _rngRateBytes, _rngRatePeriod);
+    }
 
-        final TermPolicy term = new TermPolicy();
-        term.setCrashPolicy("destroy");
-        term.setPowerOffPolicy("destroy");
-        term.setRebootPolicy("restart");
-        vm.addComp(term);
+    protected SerialDef createSerialDef() {
+        return new SerialDef(PTY, null, (short)0);
+    }
 
-        final ClockDef clock = new ClockDef();
-        if (vmTO.getOs().startsWith("Windows")) {
+    protected ClockDef createClockDef(final VirtualMachineTO vmTO) {
+        ClockDef clock = new ClockDef();
+        if (org.apache.commons.lang.StringUtils.startsWith(vmTO.getOs(), WINDOWS)) {
             clock.setClockOffset(ClockDef.ClockOffset.LOCALTIME);
-            clock.setTimer("hypervclock", null, null);
-        } else if (vmTO.getType() != VirtualMachine.Type.User || isGuestPVEnabled(vmTO.getOs())) {
-            if (_hypervisorLibvirtVersion >= 9 * 1000 + 10) {
-                clock.setTimer("kvmclock", null, null, _noKvmClock);
-            }
+            clock.setTimer(HYPERVCLOCK, null, null);
+        } else if ((vmTO.getType() != VirtualMachine.Type.User || isGuestPVEnabled(vmTO.getOs())) && _hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_MODE) {

Review comment:
       This if is quite big, maybe extract it to a method?

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));

Review comment:
       I think that `VirtualMachineTO` does not implement `toString`, in this case, the log would be something such as:
   `DPDK is enabled for VM [com.cloud.agent.api.to.VirtualMachineTO@38588dea], but it ...`
   
   Either:
   1. this log needs to be changed, or
   2. `VirtualMachineTO` needs a toString() implementation to override the `Object.toString()`

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+            addExtraConfigComponent(extraConfig, vm);
         }
+    }
 
-            vm.addComp(guest);
+    /**
+     * Add devices components to VM.
+     */
+    protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef guest, int vcpus, boolean isUefiEnabled) {
+        DevicesDef devices = new DevicesDef();
+        devices.setEmulatorPath(_hypervisorPath);
+        devices.setGuestType(guest.getGuestType());
+        devices.addDevice(createSerialDef());
 
-        final GuestResourceDef grd = new GuestResourceDef();
+        if (_rngEnable) {
+            devices.addDevice(createRngDef());
+        }
 
-        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
-            grd.setMemBalloning(true);
-            grd.setCurrentMem(vmTO.getMinRam() / 1024);
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
-        } else {
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        devices.addDevice(createChannelDef(vmTO));
+        devices.addDevice(createWatchDogDef());
+        devices.addDevice(createVideoDef());
+        devices.addDevice(createConsoleDef());
+        devices.addDevice(createGraphicDef(vmTO));
+        devices.addDevice(createTabletInputDef());
+
+        if (isGuestAarch64()) {
+            createArm64UsbDef(devices);
         }
-        final int vcpus = vmTO.getCpus();
-        grd.setVcpuNum(vcpus);
-        vm.addComp(grd);
 
-        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
-            final CpuModeDef cmd = new CpuModeDef();
-            cmd.setMode(_guestCpuMode);
-            cmd.setModel(_guestCpuModel);
-            if (vmTO.getType() == VirtualMachine.Type.User) {
-                cmd.setFeatures(_cpuFeatures);
-            }
-            setCpuTopology(cmd, vcpus, vmTO.getDetails());
-            vm.addComp(cmd);
+        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+        if (busT == null) {
+            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
+        }
+
+        if (busT == DiskDef.DiskBus.SCSI) {
+            devices.addDevice(createSCSIDef(vcpus));
         }
+        return devices;
+    }
 
-        if (_hypervisorLibvirtVersion >= 9000) {
-            final CpuTuneDef ctd = new CpuTuneDef();
-            /**
-             A 4.0.X/4.1.X management server doesn't send the correct JSON
-             command for getMinSpeed, it only sends a 'speed' field.
+    protected WatchDogDef createWatchDogDef() {
+        return new WatchDogDef(_watchDogAction, _watchDogModel);
+    }
 
-             So if getMinSpeed() returns null we fall back to getSpeed().
+    /*
+     * Add an explicit USB devices for ARM64
+     */
+    protected void createArm64UsbDef(DevicesDef devices) {
+        devices.addDevice(new InputDef(KEYBOARD, USB));
+        devices.addDevice(new InputDef(MOUSE, USB));
+        devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    }
 
-             This way a >4.1 agent can work communicate a <=4.1 management server
+    protected InputDef createTabletInputDef() {
+        return new InputDef(TABLET, USB);
+    }
 
-             This change is due to the overcommit feature in 4.2
-             */
-            if (vmTO.getMinSpeed() != null) {
-                ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
-            } else {
-                ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
-            }
+    /**
+     * Add the VNC port passwd here, get the passwd from the vmInstance.

Review comment:
       I think that this Javadoc does not entirely match with what is done (please correct me if I am wrong)
   
   `Add the VNC port passwd here, get the passwd from the vmInstance.`
   
   Suggestion :arrow_down:
   
   `Given a VM (VirtualMachineTO), it creates a Libvirt Graphic Definition with the VM's password and VNC address.`
   _OR simply:_
   `Creates a Libvirt Graphic Definition with the VM's password and VNC address.`

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+            addExtraConfigComponent(extraConfig, vm);
         }
+    }
 
-            vm.addComp(guest);
+    /**
+     * Add devices components to VM.
+     */
+    protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef guest, int vcpus, boolean isUefiEnabled) {
+        DevicesDef devices = new DevicesDef();
+        devices.setEmulatorPath(_hypervisorPath);
+        devices.setGuestType(guest.getGuestType());
+        devices.addDevice(createSerialDef());
 
-        final GuestResourceDef grd = new GuestResourceDef();
+        if (_rngEnable) {
+            devices.addDevice(createRngDef());
+        }
 
-        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
-            grd.setMemBalloning(true);
-            grd.setCurrentMem(vmTO.getMinRam() / 1024);
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
-        } else {
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        devices.addDevice(createChannelDef(vmTO));
+        devices.addDevice(createWatchDogDef());
+        devices.addDevice(createVideoDef());
+        devices.addDevice(createConsoleDef());
+        devices.addDevice(createGraphicDef(vmTO));
+        devices.addDevice(createTabletInputDef());
+
+        if (isGuestAarch64()) {
+            createArm64UsbDef(devices);
         }
-        final int vcpus = vmTO.getCpus();
-        grd.setVcpuNum(vcpus);
-        vm.addComp(grd);
 
-        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
-            final CpuModeDef cmd = new CpuModeDef();
-            cmd.setMode(_guestCpuMode);
-            cmd.setModel(_guestCpuModel);
-            if (vmTO.getType() == VirtualMachine.Type.User) {
-                cmd.setFeatures(_cpuFeatures);
-            }
-            setCpuTopology(cmd, vcpus, vmTO.getDetails());
-            vm.addComp(cmd);
+        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+        if (busT == null) {
+            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
+        }
+
+        if (busT == DiskDef.DiskBus.SCSI) {
+            devices.addDevice(createSCSIDef(vcpus));
         }
+        return devices;
+    }
 
-        if (_hypervisorLibvirtVersion >= 9000) {
-            final CpuTuneDef ctd = new CpuTuneDef();
-            /**
-             A 4.0.X/4.1.X management server doesn't send the correct JSON
-             command for getMinSpeed, it only sends a 'speed' field.
+    protected WatchDogDef createWatchDogDef() {
+        return new WatchDogDef(_watchDogAction, _watchDogModel);
+    }
 
-             So if getMinSpeed() returns null we fall back to getSpeed().
+    /*
+     * Add an explicit USB devices for ARM64
+     */
+    protected void createArm64UsbDef(DevicesDef devices) {
+        devices.addDevice(new InputDef(KEYBOARD, USB));
+        devices.addDevice(new InputDef(MOUSE, USB));
+        devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    }
 
-             This way a >4.1 agent can work communicate a <=4.1 management server
+    protected InputDef createTabletInputDef() {
+        return new InputDef(TABLET, USB);
+    }
 
-             This change is due to the overcommit feature in 4.2
-             */
-            if (vmTO.getMinSpeed() != null) {
-                ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
-            } else {
-                ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
-            }
+    /**
+     * Add the VNC port passwd here, get the passwd from the vmInstance.
+     */
+    protected GraphicDef createGraphicDef(VirtualMachineTO vmTO) {
+        return new GraphicDef(VNC, (short)0, true, vmTO.getVncAddr(), vmTO.getVncPassword(), null);
+    }
 
-            setQuotaAndPeriod(vmTO, ctd);
+    /**
+     * Add a VirtIO channel for the Qemu Guest Agent tools.

Review comment:
       `VirtIO` -> `Virtio`
   
   See:
   - https://wiki.libvirt.org/page/Virtio
   - https://libvirt.org/formatdomain.html#virtio-transitional-devices

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -209,6 +209,67 @@
 public class LibvirtComputingResource extends ServerResourceBase implements ServerResource, VirtualRouterDeployer {
     private static final Logger s_logger = Logger.getLogger(LibvirtComputingResource.class);
 
+    private static final String LEGACY = "legacy";
+    private static final String SECURE = "secure";
+
+    /**
+     * Machine type.
+     */
+    private static final String PC = "pc";
+    private static final String VIRT = "virt";
+
+    /**
+     * Possible devices to add to VM.
+     */
+    private static final String TABLET = "tablet";
+    private static final String USB = "usb";
+    private static final String MOUSE = "mouse";
+    private static final String KEYBOARD = "keyboard";
+
+    /**
+     * Policies used by VM.
+     */
+    private static final String RESTART = "restart";
+    private static final String DESTROY = "destroy";
+
+    private static final String KVMCLOCK = "kvmclock";
+    private static final String HYPERVCLOCK = "hypervclock";
+    private static final String WINDOWS = "Windows";
+    private static final String Q35 = "q35";
+    private static final String PTY = "pty";
+    private static final String VNC = "vnc";
+
+    /**
+     * Acronym of System Management Mode. Perform low-level system management operations while an OS is running.
+     */
+    private static final String SMM = "smm";
+    /**
+     * Acronym of Advanced Configuration and Power Interface.<br>
+     * Provides an open standard that operating systems can use to discover and configure
+     * computer hardware components, to perform power management.
+     */
+    private static final String ACPI = "acpi";
+    /**
+     * Acronym of Advanced Programmable Interrupt Controllers.<br>
+     * With an I/O APIC, operating systems can use more than 16 interrupt requests (IRQs)
+     * and therefore avoid IRQ sharing for improved reliability.
+     */
+    private static final String APIC = "apic";
+    /**
+     * Acronym of Physical Address Extension. Feature implemented in modern x86 processors.<br>
+     * PAE extends memory addressing capabilities, allowing more than 4 GB of random access memory (RAM) to be used.
+     */
+    private static final String PAE = "pae";
+    /**
+     * Libvirt only suport guest cpu mode in versions from 0.9.10.
+     */
+    private static final int MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_MODE = 9010;
+    private static final int MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE = 9000;

Review comment:
           /**
        * The CPU tune element provides details of the CPU tunable parameters for the domain.<br>
        * It is supported since Libvirt 0.9.0
        */

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+            addExtraConfigComponent(extraConfig, vm);
         }
+    }
 
-            vm.addComp(guest);
+    /**
+     * Add devices components to VM.
+     */
+    protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef guest, int vcpus, boolean isUefiEnabled) {
+        DevicesDef devices = new DevicesDef();
+        devices.setEmulatorPath(_hypervisorPath);
+        devices.setGuestType(guest.getGuestType());
+        devices.addDevice(createSerialDef());
 
-        final GuestResourceDef grd = new GuestResourceDef();
+        if (_rngEnable) {
+            devices.addDevice(createRngDef());
+        }
 
-        if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
-            grd.setMemBalloning(true);
-            grd.setCurrentMem(vmTO.getMinRam() / 1024);
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
-        } else {
-            grd.setMemorySize(vmTO.getMaxRam() / 1024);
+        devices.addDevice(createChannelDef(vmTO));
+        devices.addDevice(createWatchDogDef());
+        devices.addDevice(createVideoDef());
+        devices.addDevice(createConsoleDef());
+        devices.addDevice(createGraphicDef(vmTO));
+        devices.addDevice(createTabletInputDef());
+
+        if (isGuestAarch64()) {
+            createArm64UsbDef(devices);
         }
-        final int vcpus = vmTO.getCpus();
-        grd.setVcpuNum(vcpus);
-        vm.addComp(grd);
 
-        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
-            final CpuModeDef cmd = new CpuModeDef();
-            cmd.setMode(_guestCpuMode);
-            cmd.setModel(_guestCpuModel);
-            if (vmTO.getType() == VirtualMachine.Type.User) {
-                cmd.setFeatures(_cpuFeatures);
-            }
-            setCpuTopology(cmd, vcpus, vmTO.getDetails());
-            vm.addComp(cmd);
+        DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+        if (busT == null) {
+            busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
+        }
+
+        if (busT == DiskDef.DiskBus.SCSI) {
+            devices.addDevice(createSCSIDef(vcpus));
         }
+        return devices;
+    }
 
-        if (_hypervisorLibvirtVersion >= 9000) {
-            final CpuTuneDef ctd = new CpuTuneDef();
-            /**
-             A 4.0.X/4.1.X management server doesn't send the correct JSON
-             command for getMinSpeed, it only sends a 'speed' field.
+    protected WatchDogDef createWatchDogDef() {
+        return new WatchDogDef(_watchDogAction, _watchDogModel);
+    }
 
-             So if getMinSpeed() returns null we fall back to getSpeed().
+    /*
+     * Add an explicit USB devices for ARM64
+     */
+    protected void createArm64UsbDef(DevicesDef devices) {
+        devices.addDevice(new InputDef(KEYBOARD, USB));
+        devices.addDevice(new InputDef(MOUSE, USB));
+        devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+    }
 
-             This way a >4.1 agent can work communicate a <=4.1 management server
+    protected InputDef createTabletInputDef() {
+        return new InputDef(TABLET, USB);
+    }
 
-             This change is due to the overcommit feature in 4.2
-             */
-            if (vmTO.getMinSpeed() != null) {
-                ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
-            } else {
-                ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
-            }
+    /**
+     * Add the VNC port passwd here, get the passwd from the vmInstance.
+     */
+    protected GraphicDef createGraphicDef(VirtualMachineTO vmTO) {
+        return new GraphicDef(VNC, (short)0, true, vmTO.getVncAddr(), vmTO.getVncPassword(), null);
+    }
 
-            setQuotaAndPeriod(vmTO, ctd);
+    /**
+     * Add a VirtIO channel for the Qemu Guest Agent tools.
+     */
+    protected ChannelDef createChannelDef(VirtualMachineTO vmTO) {
+        File virtIoChannel = Paths.get(_qemuSocketsPath.getPath(), vmTO.getName() + "." + _qemuGuestAgentSocketName).toFile();
+        return new ChannelDef(_qemuGuestAgentSocketName, ChannelDef.ChannelType.UNIX, virtIoChannel);
+    }
 
-            vm.addComp(ctd);
-        }
+    /**
+     * If we're using virtio scsi, then we need to add a virtual scsi controller.
+     */

Review comment:
       I would suggest changing it to:
   ```
       /**
        * Creates Virtio SCSI controller.
        */
   ```
        
   I am not sure if it is relevant to mention `If we're using virtio scsi, then we need to add a virtual scsi controller`. The method logic is restricted to creating a SCSI XML definition.
   
   The logic that defines if it does create (or not) such definition is:
        ~~~
           if (busT == DiskDef.DiskBus.SCSI) {
               devices.addDevice(createSCSIDef(vcpus));
           }
        ~~~
        
   But if you think it still relevant, I would then suggest adding the respective SCSI validation to this method.
   
   In such case, here follows a suggestion (if you think it is better) :-)
   ```
       /**
        * Creates Virtio SCSI controller. <br>
        * The respective Virtio SCSI XML definition is generated only if the VM's Disk Bus is of ISCSI.
        */
   ```

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
         Map<String, String> customParams = vmTO.getDetails();
         boolean isUefiEnabled = false;
         boolean isSecureBoot = false;
-        String bootMode =null;
+        String bootMode = null;
+
         if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
             isUefiEnabled = true;
-            bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
-            if (StringUtils.isNotBlank(bootMode) && "secure".equalsIgnoreCase(bootMode)) {
+            s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].", uuid));
+
+            if (isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+                s_logger.debug(String.format("Enabled Secure Boot for VM UUID [%s].", uuid));
                 isSecureBoot = true;
             }
         }
 
         Map<String, String> extraConfig = vmTO.getExtraConfig();
         if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) || !extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
-            s_logger.info("DPDK is enabled but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment");
+            s_logger.info(String.format("DPDK is enabled for VM [%s], but it needs extra configurations for CPU NUMA and Huge Pages for VM deployment.", vmTO.toString()));
         }
+        configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot, bootMode, extraConfig, uuid);
+        return vm;
+    }
 
-        final GuestDef guest = new GuestDef();
+    /**
+     * Configure created VM from specification, adding the necessary components to VM.
+     */
+    private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot, String bootMode,
+            Map<String, String> extraConfig, String uuid) {
+        s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
 
-        if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User == vmTO.getType()) {
-            // LXC domain is only valid for user VMs. Use KVM for system VMs.
-            guest.setGuestType(GuestDef.GuestType.LXC);
-            vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
-        } else {
-            guest.setGuestType(GuestDef.GuestType.KVM);
-            vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
-            vm.setLibvirtVersion(_hypervisorLibvirtVersion);
-            vm.setQemuVersion(_hypervisorQemuVersion);
+        GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+        if (isUefiEnabled) {
+            configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
         }
-        guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch : vmTO.getArch());
-        guest.setMachineType(_guestCpuArch != null && _guestCpuArch.equals("aarch64") ? "virt" : "pc");
-        guest.setBootType(GuestDef.BootType.BIOS);
-        if (MapUtils.isNotEmpty(customParams) && customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
-            guest.setBootType(GuestDef.BootType.UEFI);
-            guest.setBootMode(GuestDef.BootMode.LEGACY);
-            guest.setMachineType("q35");
-            if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) {
-                guest.setBootMode(GuestDef.BootMode.SECURE); // setting to secure mode
-            }
+
+        vm.addComp(guest);
+        vm.addComp(createGuestResourceDef(vmTO));
+
+        int vcpus = vmTO.getCpus();
+        if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+            vm.addComp(createCpuModeDef(vmTO, vcpus));
         }
-        guest.setUuid(uuid);
-        guest.setBootOrder(GuestDef.BootOrder.CDROM);
-        guest.setBootOrder(GuestDef.BootOrder.HARDISK);
 
-        if (isUefiEnabled) {
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
-            }
+        if (_hypervisorLibvirtVersion >= MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+            vm.addComp(createCpuTuneDef(vmTO));
+        }
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) != null && "legacy".equalsIgnoreCase(bootMode)) {
-                guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
-            }
+        FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled, isSecureBoot);
+        enlightenWindowsVm(vmTO, features);
+        vm.addComp(features);
 
-            if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) != null) {
-                guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
-            }
+        vm.addComp(createTermPolicy());
+        vm.addComp(createClockDef(vmTO));
+        vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
 
-            if (isSecureBoot) {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null && "secure".equalsIgnoreCase(bootMode)) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
-                }
-            } else {
-                if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-                    guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
-                }
-            }
+        addExtraConfigsToVM(vmTO, vm, extraConfig);
+    }
+
+    /**
+     *  Add extra configuration to User VM Domain XML before starting.
+     */
+    private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm, Map<String, String> extraConfig) {
+        if (MapUtils.isNotEmpty(extraConfig) && VirtualMachine.Type.User.equals(vmTO.getType())) {
+            s_logger.debug(String.format("Appending extra configuration data [%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));

Review comment:
       Another occurrence of `vmTO.toString()`, it needs to be checked.

##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -209,6 +209,67 @@
 public class LibvirtComputingResource extends ServerResourceBase implements ServerResource, VirtualRouterDeployer {
     private static final Logger s_logger = Logger.getLogger(LibvirtComputingResource.class);
 
+    private static final String LEGACY = "legacy";
+    private static final String SECURE = "secure";
+
+    /**
+     * Machine type.
+     */
+    private static final String PC = "pc";
+    private static final String VIRT = "virt";
+
+    /**
+     * Possible devices to add to VM.
+     */
+    private static final String TABLET = "tablet";
+    private static final String USB = "usb";
+    private static final String MOUSE = "mouse";
+    private static final String KEYBOARD = "keyboard";
+
+    /**
+     * Policies used by VM.
+     */
+    private static final String RESTART = "restart";
+    private static final String DESTROY = "destroy";
+
+    private static final String KVMCLOCK = "kvmclock";
+    private static final String HYPERVCLOCK = "hypervclock";
+    private static final String WINDOWS = "Windows";
+    private static final String Q35 = "q35";
+    private static final String PTY = "pty";
+    private static final String VNC = "vnc";
+
+    /**
+     * Acronym of System Management Mode. Perform low-level system management operations while an OS is running.
+     */
+    private static final String SMM = "smm";
+    /**
+     * Acronym of Advanced Configuration and Power Interface.<br>
+     * Provides an open standard that operating systems can use to discover and configure
+     * computer hardware components, to perform power management.
+     */
+    private static final String ACPI = "acpi";
+    /**
+     * Acronym of Advanced Programmable Interrupt Controllers.<br>
+     * With an I/O APIC, operating systems can use more than 16 interrupt requests (IRQs)
+     * and therefore avoid IRQ sharing for improved reliability.
+     */
+    private static final String APIC = "apic";
+    /**
+     * Acronym of Physical Address Extension. Feature implemented in modern x86 processors.<br>
+     * PAE extends memory addressing capabilities, allowing more than 4 GB of random access memory (RAM) to be used.
+     */
+    private static final String PAE = "pae";
+    /**
+     * Libvirt only suport guest cpu mode in versions from 0.9.10.

Review comment:
       Libvirt only suport guest cpu mode in versions from 0.9.10.
   :arrow_down: 
   Libvirt **supports** guest **CPU** mode since 0.9.10.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org