Avoid setting cpu element on unsupported architectures (#1633)

The CPU element to manage the mode, model, features (including nested),
is only available on some architectures. To allow this plugin to
generate XML valid for other architectures such as RISC-V, the CPU
element needs to be optional and only enabled when the architecture
specified supports it.

Include checks in the validation section to help prevent the setting of
an unsupported architecture with any of the CPU features that require
the CPU element to be available.

Fixes: #1538

lib/vagrant-libvirt/action/start_domain.rb

@@ -87,79 +87,83 @@ module VagrantPlugins
           end
 
           # cpu_mode
-          cpu = REXML::XPath.first(xml_descr, '/domain/cpu')
-          if cpu.nil?
-            @logger.debug "cpu_mode updated from not set to '#{config.cpu_mode}'"
-            descr_changed = true
-            cpu = REXML::Element.new('cpu', REXML::XPath.first(xml_descr, '/domain'))
-            cpu.attributes['mode'] = config.cpu_mode
-          else
-            if cpu.attributes['mode'] != config.cpu_mode
-              @logger.debug "cpu_mode updated from '#{cpu.attributes['mode']}' to '#{config.cpu_mode}'"
+          if !config.cpu_mode.nil?
+            cpu = REXML::XPath.first(xml_descr, '/domain/cpu')
+            if cpu.nil?
+              @logger.debug "cpu_mode updated from not set to '#{config.cpu_mode}'"
               descr_changed = true
+              cpu = REXML::Element.new('cpu', REXML::XPath.first(xml_descr, '/domain'))
               cpu.attributes['mode'] = config.cpu_mode
+            else
+              if cpu.attributes['mode'] != config.cpu_mode
+                @logger.debug "cpu_mode updated from '#{cpu.attributes['mode']}' to '#{config.cpu_mode}'"
+                descr_changed = true
+                cpu.attributes['mode'] = config.cpu_mode
+              end
             end
-          end
 
-          if config.cpu_mode != 'host-passthrough'
-            cpu_model = REXML::XPath.first(xml_descr, '/domain/cpu/model')
-            if cpu_model.nil?
-              if config.cpu_model.strip != ''
-                @logger.debug "cpu_model updated from not set to '#{config.cpu_model}'"
-                descr_changed = true
-                cpu_model = REXML::Element.new('model', REXML::XPath.first(xml_descr, '/domain/cpu'))
-                cpu_model.attributes['fallback'] = 'allow'
-                cpu_model.text = config.cpu_model
+            if config.cpu_mode != 'host-passthrough'
+              cpu_model = REXML::XPath.first(xml_descr, '/domain/cpu/model')
+              if cpu_model.nil?
+                if config.cpu_model.strip != ''
+                  @logger.debug "cpu_model updated from not set to '#{config.cpu_model}'"
+                  descr_changed = true
+                  cpu_model = REXML::Element.new('model', REXML::XPath.first(xml_descr, '/domain/cpu'))
+                  cpu_model.attributes['fallback'] = config.cpu_fallback
+                  cpu_model.text = config.cpu_model
+                end
+              else
+                if (cpu_model.text or '').strip != config.cpu_model.strip
+                  @logger.debug "cpu_model text updated from #{cpu_model.text} to '#{config.cpu_model}'"
+                  descr_changed = true
+                  cpu_model.text = config.cpu_model
+                end
+                if cpu_model.attributes['fallback'] != config.cpu_fallback
+                  @logger.debug "cpu_model fallback attribute updated from #{cpu_model.attributes['fallback']} to '#{config.cpu_fallback}'"
+                  descr_changed = true
+                  cpu_model.attributes['fallback'] = config.cpu_fallback
+                end
               end
-            else
-              if (cpu_model.text or '').strip != config.cpu_model.strip
-                @logger.debug "cpu_model text updated from #{cpu_model.text} to '#{config.cpu_model}'"
-                descr_changed = true
-                cpu_model.text = config.cpu_model
+              vmx_feature = REXML::XPath.first(xml_descr, '/domain/cpu/feature[@name="vmx"]')
+              svm_feature = REXML::XPath.first(xml_descr, '/domain/cpu/feature[@name="svm"]')
+              if config.nested
+                if vmx_feature.nil?
+                  @logger.debug "nested mode enabled from unset by setting cpu vmx feature"
+                  descr_changed = true
+                  vmx_feature = REXML::Element.new('feature', REXML::XPath.first(xml_descr, '/domain/cpu'))
+                  vmx_feature.attributes['policy'] = 'optional'
+                  vmx_feature.attributes['name'] = 'vmx'
+                end
+                if svm_feature.nil?
+                  @logger.debug "nested mode enabled from unset by setting cpu svm feature"
+                  descr_changed = true
+                  svm_feature = REXML::Element.new('feature', REXML::XPath.first(xml_descr, '/domain/cpu'))
+                  svm_feature.attributes['policy'] = 'optional'
+                  svm_feature.attributes['name'] = 'svm'
+                end
+              else
+                unless vmx_feature.nil?
+                  @logger.debug "nested mode disabled for cpu by removing vmx feature"
+                  descr_changed = true
+                  cpu.delete_element(vmx_feature)
+                end
+                unless svm_feature.nil?
+                  @logger.debug "nested mode disabled for cpu by removing svm feature"
+                  descr_changed = true
+                  cpu.delete_element(svm_feature)
+                end
               end
-              if cpu_model.attributes['fallback'] != config.cpu_fallback
-                @logger.debug "cpu_model fallback attribute updated from #{cpu_model.attributes['fallback']} to '#{config.cpu_fallback}'"
+            elsif config.numa_nodes == nil
+              unless cpu.elements.to_a.empty?
+                @logger.debug "switching cpu_mode to host-passthrough and removing emulated cpu features"
                 descr_changed = true
-                cpu_model.attributes['fallback'] = config.cpu_fallback
-              end
-            end
-            vmx_feature = REXML::XPath.first(xml_descr, '/domain/cpu/feature[@name="vmx"]')
-            svm_feature = REXML::XPath.first(xml_descr, '/domain/cpu/feature[@name="svm"]')
-            if config.nested
-              if vmx_feature.nil?
-                @logger.debug "nested mode enabled from unset by setting cpu vmx feature"
-                descr_changed = true
-                vmx_feature = REXML::Element.new('feature', REXML::XPath.first(xml_descr, '/domain/cpu'))
-                vmx_feature.attributes['policy'] = 'optional'
-                vmx_feature.attributes['name'] = 'vmx'
-              end
-              if svm_feature.nil?
-                @logger.debug "nested mode enabled from unset by setting cpu svm feature"
-                descr_changed = true
-                svm_feature = REXML::Element.new('feature', REXML::XPath.first(xml_descr, '/domain/cpu'))
-                svm_feature.attributes['policy'] = 'optional'
-                svm_feature.attributes['name'] = 'svm'
-              end
-            else
-              unless vmx_feature.nil?
-                @logger.debug "nested mode disabled for cpu by removing vmx feature"
-                descr_changed = true
-                cpu.delete_element(vmx_feature)
-              end
-              unless svm_feature.nil?
-                @logger.debug "nested mode disabled for cpu by removing svm feature"
-                descr_changed = true
-                cpu.delete_element(svm_feature)
-              end
-            end
-          elsif config.numa_nodes == nil
-            unless cpu.elements.to_a.empty?
-              @logger.debug "switching cpu_mode to host-passthrough and removing emulated cpu features"
-              descr_changed = true
-              cpu.elements.each do |elem|
-                cpu.delete_element(elem)
+                cpu.elements.each do |elem|
+                  cpu.delete_element(elem)
+                end
               end
             end
+          else
+            xml_descr.delete_element('/domain/cpu')
           end
 
           # Clock

lib/vagrant-libvirt/config.rb

@@ -208,6 +208,14 @@ module VagrantPlugins
       # internal helper attributes
       attr_accessor :host_device_exclude_prefixes
 
+      # list of architectures that support cpu based on https://github.com/libvirt/libvirt/tree/master/src/cpu
+      ARCH_SUPPORT_CPU = [
+        'aarch64', 'armv6l', 'armv7b', 'armv7l',
+        'i686', 'x86_64',
+        'ppc64', 'ppc64le',
+        's390', 's390x',
+      ]
+
       def initialize
         @uri               = UNSET_VALUE
         @driver            = UNSET_VALUE
@@ -913,12 +921,23 @@ module VagrantPlugins
         @title = '' if @title == UNSET_VALUE
         @description = '' if @description == UNSET_VALUE
         @uuid = '' if @uuid == UNSET_VALUE
+        @machine_type = nil if @machine_type == UNSET_VALUE
+        @machine_arch = nil if @machine_arch == UNSET_VALUE
         @memory = 512 if @memory == UNSET_VALUE
         @nodeset = nil if @nodeset == UNSET_VALUE
         @memory_backing = [] if @memory_backing == UNSET_VALUE
         @cpus = 1 if @cpus == UNSET_VALUE
         @cpuset = nil if @cpuset == UNSET_VALUE
-        @cpu_mode = 'host-model' if @cpu_mode == UNSET_VALUE
+        @cpu_mode = if @cpu_mode == UNSET_VALUE
+                      # only some architectures support the cpu element
+                      if @machine_arch.nil? || ARCH_SUPPORT_CPU.include?(@machine_arch.downcase)
+                        'host-model'
+                      else
+                        nil
+                      end
+                    else
+                      @cpu_mode
+                    end
         @cpu_model = if (@cpu_model == UNSET_VALUE) && (@cpu_mode == 'custom')
                        'qemu64'
                      elsif @cpu_mode != 'custom'
@@ -938,8 +957,6 @@ module VagrantPlugins
         @numa_nodes = @numa_nodes == UNSET_VALUE ? nil : _generate_numa
         @loader = nil if @loader == UNSET_VALUE
         @nvram = nil if @nvram == UNSET_VALUE
-        @machine_type = nil if @machine_type == UNSET_VALUE
-        @machine_arch = nil if @machine_arch == UNSET_VALUE
         @machine_virtual_size = nil if @machine_virtual_size == UNSET_VALUE
         @disk_device = @disk_bus == 'scsi' ? 'sda' : 'vda' if @disk_device == UNSET_VALUE
         @disk_bus = @disk_device.start_with?('sd') ? 'scsi' : 'virtio' if @disk_bus == UNSET_VALUE
@@ -1071,6 +1088,22 @@ module VagrantPlugins
       def validate(machine)
         errors = _detected_errors
 
+        unless @machine_arch.nil? || ARCH_SUPPORT_CPU.include?(@machine_arch.downcase)
+          unsupported = [:cpu_mode, :cpu_model, :nested, :cpu_features, :cpu_topology, :numa_nodes]
+          cpu_support_required_by = unsupported.select { |x|
+            value = instance_variable_get("@#{x.to_s}")
+            next if value.nil?  # not set
+            is_bool = !!value == value
+            next if is_bool && !value  # boolean and set to false
+            next if !is_bool && value.empty?  # not boolean, but empty '', [], {}
+            true
+          }
+
+          unless cpu_support_required_by.empty?
+            errors << "Architecture #{@machine_arch} does not support /domain/cpu XML, which is required when setting the config options #{cpu_support_required_by.join(", ")}"
+          end
+        end
+
         # technically this shouldn't occur, but ensure that if somehow it does, it gets rejected.
         if @cpu_mode == 'host-passthrough' && @cpu_model != ''
           errors << "cannot set cpu_model with cpu_mode of 'host-passthrough'. leave model unset or switch mode."

lib/vagrant-libvirt/templates/domain.xml.erb

@@ -5,33 +5,35 @@
   <uuid><%= @uuid %></uuid>
   <memory><%= @memory_size %></memory>
   <vcpu<% if @cpuset %> cpuset='<%= @cpuset %>'<% end %>><%= @cpus %></vcpu>
+<%- unless @cpu_mode.nil? -%>
   <cpu mode='<%= @cpu_mode %>'>
-<%- if @cpu_mode != 'host-passthrough' -%>
+  <%- if @cpu_mode != 'host-passthrough' -%>
     <model fallback='<%= @cpu_fallback %>'><% if @cpu_mode == 'custom' %><%= @cpu_model %><% end %></model>
-<%- end -%>
-<%- if @nested -%>
-  <%- if @cpu_features.select{|x| x[:name] == 'vmx'}.empty? -%>
+  <%- end -%>
+  <%- if @nested -%>
+    <%- if @cpu_features.select{|x| x[:name] == 'vmx'}.empty? -%>
     <feature policy='optional' name='vmx'/>
-  <%- end -%>
-  <%- if @cpu_features.select{|x| x[:name] == 'svm'}.empty? -%>
+    <%- end -%>
+    <%- if @cpu_features.select{|x| x[:name] == 'svm'}.empty? -%>
     <feature policy='optional' name='svm'/>
+    <%- end -%>
   <%- end -%>
-<%- end -%>
-<%- @cpu_features.each do |cpu_feature| -%>
+  <%- @cpu_features.each do |cpu_feature| -%>
     <feature name='<%= cpu_feature[:name] %>' policy='<%= cpu_feature[:policy] %>'/>
-<%- end -%>
-<%- unless @cpu_topology.empty? -%>
+  <%- end -%>
+  <%- unless @cpu_topology.empty? -%>
     <%# CPU topology -%>
     <topology sockets='<%= @cpu_topology[:sockets] %>' cores='<%= @cpu_topology[:cores] %>' threads='<%= @cpu_topology[:threads] %>'/>
-<%- end -%>
-<%- if @numa_nodes -%>
-    <numa>
-  <%- @numa_nodes.each_with_index do |node, index| -%>
-      <cell id='<%= index %>' cpus='<%= node[:cpus] %>' memory='<%= node[:memory] %>'<% if node.key?(:memAccess) %> memAccess='<%= node[:memAccess] %>'<% end %>/>
   <%- end -%>
+  <%- if @numa_nodes -%>
+    <numa>
+    <%- @numa_nodes.each_with_index do |node, index| -%>
+      <cell id='<%= index %>' cpus='<%= node[:cpus] %>' memory='<%= node[:memory] %>'<% if node.key?(:memAccess) %> memAccess='<%= node[:memAccess] %>'<% end %>/>
+    <%- end -%>
     </numa>
-<%- end -%>
+  <%- end -%>
   </cpu>
+<%- end -%>
 <%- if @nodeset -%>
   <numatune>
     <memory nodeset='<%= @nodeset %>'/>

spec/unit/action/start_domain_spec.rb

@@ -161,6 +161,71 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
       end
     end
 
+    context 'cpu' do
+      let(:test_file) { 'existing.xml' }
+      let(:updated_domain_xml) {
+        new_xml = domain_xml.dup
+        new_xml.gsub!(
+          /<cpu .*\/>/,
+          <<-EOF
+          <cpu check='partial' mode='custom'>
+            <model fallback='allow'>Haswell</model>
+            <feature name='vmx' policy='optional'/>
+            <feature name='svm' policy='optional'/>
+          </cpu>
+          EOF
+        )
+        new_xml
+      }
+      let(:vagrantfile_providerconfig) {
+        <<-EOF
+        libvirt.cpu_mode = 'custom'
+        libvirt.cpu_model = 'Haswell'
+        libvirt.nested = true
+        EOF
+      }
+
+      it 'should set cpu related settings when changed' do
+        expect(ui).to_not receive(:warn)
+        expect(connection).to receive(:define_domain).and_return(libvirt_domain)
+        expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
+        expect(libvirt_domain).to receive(:autostart=)
+        expect(domain).to receive(:start)
+
+        expect(subject.call(env)).to be_nil
+      end
+
+      let(:domain_xml_no_cpu) {
+        new_xml = domain_xml.dup
+        new_xml.gsub!(/<cpu .*\/>/, '')
+        new_xml
+      }
+      let(:updated_domain_xml_new_cpu) {
+        new_xml = domain_xml.dup
+        new_xml.gsub!(
+          /<cpu .*\/>/,
+          <<-EOF
+          <cpu mode='custom'>
+            <model fallback='allow'>Haswell</model>
+            <feature name='vmx' policy='optional'/>
+            <feature name='svm' policy='optional'/>
+          </cpu>
+          EOF
+        )
+        new_xml
+      }
+
+      it 'should add cpu settings if not already present' do
+        expect(ui).to_not receive(:warn)
+        expect(connection).to receive(:define_domain).and_return(libvirt_domain)
+        expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml_no_cpu, updated_domain_xml_new_cpu)
+        expect(libvirt_domain).to receive(:autostart=)
+        expect(domain).to receive(:start)
+
+        expect(subject.call(env)).to be_nil
+      end
+    end
+
     context 'graphics' do
       context 'autoport not disabled' do
         let(:test_file) { 'existing.xml' }

spec/unit/config_spec.rb

@@ -676,6 +676,33 @@ describe VagrantPlugins::ProviderLibvirt::Config do
         expect(subject.channels).to match([a_hash_including({:target_name => 'com.redhat.spice.0'})])
       end
     end
+
+    context '@machine_arch and @cpu_*' do
+      context 'should set @cpu_mode based on @machine_arch support' do
+        # it's possible when this is unset that the host arch should be read
+        it 'should default to host-model if machine_arch unset' do
+          subject.finalize!
+
+          expect(subject.cpu_mode).to eq('host-model')
+        end
+
+        it 'should default to host-model if supported' do
+          subject.machine_arch = 'aarch64'
+
+          subject.finalize!
+
+          expect(subject.cpu_mode).to eq('host-model')
+        end
+
+        it 'should default to nil if unsupported' do
+          subject.machine_arch = 'ppc'
+
+          subject.finalize!
+
+          expect(subject.cpu_mode).to be_nil
+        end
+      end
+    end
   end
 
   def assert_invalid
@@ -804,6 +831,32 @@ describe VagrantPlugins::ProviderLibvirt::Config do
       end
     end
 
+    context '@machine_arch and @cpu_*' do
+      it 'should be valid if cpu_* settings and no arch set' do
+        subject.cpu_mode = 'host-passthrough'
+        subject.nested = true
+
+        assert_valid
+      end
+
+      it 'should be valid if cpu_* settings and supported' do
+        subject.machine_arch = 'aarch64'
+        subject.cpu_mode = 'host-passthrough'
+        subject.nested = true
+
+        assert_valid
+      end
+
+      it 'should flag settings invalid if unsupported' do
+        subject.machine_arch = 'ppc'
+        subject.cpu_mode = 'host-passthrough'
+        subject.nested = true
+
+        errors = assert_invalid
+        expect(errors).to include(match(/Architecture ppc does not support .* cpu_mode, nested/))
+      end
+    end
+
     context 'with cpu_mode and cpu_model defined' do
       it 'should discard model if mode is passthrough' do
         subject.cpu_mode = 'host-passthrough'