Rewrite PCI attaching/detaching from xl to libvirt

The only remaining part is querying vm-side BDF. That can't be done
in libvirt.

ci/pylintrc

@@ -30,7 +30,8 @@ reports=yes
 ignored-classes=
   VMProperty,
   libvirt,libvirtError,
-  dbus,SystemBus
+  dbus,SystemBus,
+  PCIDevice
 
 ignore-mixin-members=yes
 generated-members=

qubes/devices.py

@@ -130,6 +130,10 @@ class PCIDevice(RegexDevice):
     regex = re.compile(
         r'^(?P<bus>[0-9a-f]+):(?P<device>[0-9a-f]+)\.(?P<function>[0-9a-f]+)$')
 
+    @property
+    def libvirt_name(self):
+        return 'pci_000_{}_{}_{}'.format(self.bus, self.device, self.name)
+
 
 class BlockDevice(object):
     # pylint: disable=too-few-public-methods

qubes/vm/__init__.py

@@ -292,96 +292,6 @@ class BaseVM(qubes.PropertyHolder):
         return interface
 
 
-    @staticmethod
-    def lvxml_pci_dev(address):
-        '''Return ``<hostdev>`` node for libvirt xml.
-
-        This was previously _format_pci_dev
-
-        :param str ip: IP address of the frontend
-        :param str mac: MAC (Ethernet) address of the frontend
-        :param qubes.vm.qubesvm.QubesVM backend: Backend domain
-        :rtype: lxml.etree._Element
-        '''
-
-        dev_match = re.match(r'([0-9a-f]+):([0-9a-f]+)\.([0-9a-f]+)', address)
-        if not dev_match:
-            raise ValueError('Invalid PCI device address: {!r}'.format(address))
-
-        hostdev = lxml.etree.Element('hostdev', type='pci', managed='yes')
-        source = lxml.etree.Element('source')
-        source.append(lxml.etree.Element('address',
-            bus='0x' + dev_match.group(1),
-            slot='0x' + dev_match.group(2),
-            function='0x' + dev_match.group(3)))
-        hostdev.append(source)
-        return hostdev
-
-    #
-    # old libvirt XML
-    # TODO rewrite it to do proper XML synthesis via lxml.etree
-    #
-
-    def get_config_params(self):
-        '''Return parameters for libvirt's XML domain config
-
-        .. deprecated:: 3.0-alpha This will go away.
-        '''
-
-        args = {}
-        args['name'] = self.name
-        args['uuid'] = str(self.uuid)
-        args['vmdir'] = self.dir_path
-        args['pcidevs'] = ''.join(lxml.etree.tostring(self.lvxml_pci_dev(dev))
-            for dev in self.devices['pci'])
-        args['maxmem'] = str(self.maxmem)
-        args['vcpus'] = str(self.vcpus)
-        args['mem'] = str(min(self.memory, self.maxmem))
-
-        # If dynamic memory management disabled, set maxmem=mem
-        if not self.features.get('meminfo-writer', True):
-            args['maxmem'] = args['mem']
-
-        if self.netvm is not None:
-            args['ip'] = self.ip
-            args['mac'] = self.mac
-            args['gateway'] = self.netvm.gateway
-
-            for i, addr in zip(itertools.count(start=1), self.dns):
-                args['dns{}'.format(i)] = addr
-
-            args['netmask'] = self.netmask
-            args['netdev'] = lxml.etree.tostring(
-                self.lvxml_net_dev(self.ip, self.mac, self.netvm))
-            args['network_begin'] = ''
-            args['network_end'] = ''
-            args['no_network_begin'] = '<!--'
-            args['no_network_end'] = '-->'
-        else:
-            args['ip'] = ''
-            args['mac'] = ''
-            args['gateway'] = ''
-            args['dns1'] = ''
-            args['dns2'] = ''
-            args['netmask'] = ''
-            args['netdev'] = ''
-            args['network_begin'] = '<!--'
-            args['network_end'] = '-->'
-            args['no_network_begin'] = ''
-            args['no_network_end'] = ''
-
-        args.update(self.storage.get_config_params())
-
-        if hasattr(self, 'kernelopts'):
-            args['kernelopts'] = self.kernelopts
-            if self.debug:
-                self.log.info(
-                    "Debug mode: adding 'earlyprintk=xen' to kernel opts")
-                args['kernelopts'] += ' earlyprintk=xen'
-
-        return args
-
-
     def create_config_file(self, file_path=None, prepare_dvm=False):
         '''Create libvirt's XML domain config file
 

qubes/vm/qubesvm.py

@@ -625,49 +625,82 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
 
 
     @qubes.events.handler('device-pre-attach:pci')
-    def on_device_pre_attached_pci(self, event, pci):
+    def on_device_pre_attached_pci(self, event, device):
         # pylint: disable=unused-argument
-        if not os.path.exists('/sys/bus/pci/devices/0000:{}'.format(pci)):
+        if not os.path.exists('/sys/bus/pci/devices/0000:{}'.format(device)):
-            raise qubes.exc.QubesException('Invalid PCI device: {}'.format(pci))
+            raise qubes.exc.QubesException(
+                'Invalid PCI device: {}'.format(device))
 
         if not self.is_running():
             return
 
         try:
-            # TODO: libvirt-ise
+            self.bind_pci_to_pciback(device)
-            subprocess.check_call(
+            self.libvirt_domain.attachDevice(
-                ['sudo', qubes.config.system_path['qubes_pciback_cmd'], pci])
+                self.app.env.get_template('libvirt/devices/pci.xml').render(
-            subprocess.check_call(
+                    device=device))
-                ['sudo', 'xl', 'pci-attach', str(self.xid), pci])
         except subprocess.CalledProcessError as e:
             self.log.exception('Failed to attach PCI device {!r} on the fly,'
-                ' changes will be seen after VM restart.'.format(pci), e)
+                ' changes will be seen after VM restart.'.format(device), e)
 
 
     @qubes.events.handler('device-pre-detach:pci')
-    def on_device_pre_detached_pci(self, event, pci):
+    def on_device_pre_detached_pci(self, event, device):
         # pylint: disable=unused-argument
         if not self.is_running():
             return
 
-        # TODO: libvirt-ise
+        # this cannot be converted to general API, because there is no
+        # provision in libvirt for extracting device-side BDF; we need it for
+        # qubes.DetachPciDevice, which unbinds driver, not to oops the kernel
+
         p = subprocess.Popen(['xl', 'pci-list', str(self.xid)],
                 stdout=subprocess.PIPE)
-        result = p.communicate()
+        result = p.communicate()[0]
-        m = re.search(r"^(\d+.\d+)\s+0000:%s$" % pci, result[0],
+        m = re.search(r'^(\d+.\d+)\s+0000:{}$'.format(device), result,
             flags=re.MULTILINE)
         if not m:
-            print >>sys.stderr, "Device %s already detached" % pci
+            self.log.error('Device %s already detached', device)
             return
         vmdev = m.group(1)
         try:
-            self.run_service("qubes.DetachPciDevice",
+            self.run_service('qubes.DetachPciDevice',
-                user="root", input="00:%s" % vmdev)
+                user='root', input='00:{}'.format(vmdev))
-            subprocess.check_call(
+            self.libvirt_domain.detachDevice(
-                ['sudo', 'xl', 'pci-detach', str(self.xid), pci])
+                self.app.env.get_template('libvirt/devices/pci.xml').render(
-        except subprocess.CalledProcessError as e:
+                    device=device))
+        except (subprocess.CalledProcessError, libvirt.libvirtError) as e:
             self.log.exception('Failed to detach PCI device {!r} on the fly,'
-                ' changes will be seen after VM restart.'.format(pci), e)
+                ' changes will be seen after VM restart.'.format(device), e)
+            raise
+
+
+    def bind_pci_to_pciback(self, device):
+        '''Bind PCI device to pciback driver.
+
+        :param qubes.devices.PCIDevice device: device to attach
+
+        Devices should be unbound from their normal kernel drivers and bound to
+        the dummy driver, which allows for attaching them to a domain.
+        '''
+        try:
+            node = self.app.vmm.libvirt_conn.nodeDeviceLookupByName(
+                device.libvirt_name)
+        except libvirt.libvirtError as e:
+            if e.get_error_code() == libvirt.VIR_ERR_NO_NODE_DEVICE:
+                raise qubes.exc.QubesException(
+                    'PCI device {!r} does not exist (domain {!r})'.format(
+                        device, self.name))
+            raise
+
+        try:
+            node.dettach()
+        except libvirt.libvirtError as e:
+            if e.get_error_code() == libvirt.VIR_ERR_INTERNAL_ERROR:
+                # allreaddy dettached
+                pass
+            else:
+                raise
 
 
     #
@@ -709,23 +742,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
 
         # Bind pci devices to pciback driver
         for pci in self.devices['pci']:
-            try:
+            self.bind_pci_to_pciback(pci)
-                node = self.app.vmm.libvirt_conn.nodeDeviceLookupByName(
-                    'pci_0000_' + pci.replace(':', '_').replace('.', '_'))
-            except libvirt.libvirtError as e:
-                if e.get_error_code() == libvirt.VIR_ERR_NO_NODE_DEVICE:
-                    raise qubes.exc.QubesException(
-                        'PCI device {!r} does not exist (domain {!r})'.format(
-                            pci, self.name))
-
-            try:
-                node.dettach()
-            except libvirt.libvirtError as e:
-                if e.get_error_code() == libvirt.VIR_ERR_INTERNAL_ERROR:
-                    # allreaddy dettached
-                    pass
-                else:
-                    raise
 
         self.libvirt_domain.createWithFlags(libvirt.VIR_DOMAIN_START_PAUSED)
 
@@ -785,15 +802,15 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
 
         # try to gracefully detach PCI devices before shutdown, to mitigate
         # timeouts on forcible detach at domain destroy; if that fails, too bad
-        for pci in self.devices['pci']:
+        for device in self.devices['pci']:
             try:
-                self.libvirt_domain.detachDevice(
+                self.libvirt_domain.detachDevice(self.app.env.get_template(
-                    lxml.etree.tostring(self.lvxml_pci_dev(pci)))
+                    'libvirt/devices/pci.xml').render(device=device))
             except libvirt.libvirtError as e:
                 self.log.warning(
                     'error while gracefully detaching PCI device ({!r}) during'
                     ' shutdown of {!r}; error code: {!r}; continuing'
-                    ' anyway'.format(pci, self.name, e.get_error_code()),
+                    ' anyway'.format(device, self.name, e.get_error_code()),
                     exc_info=1)
 
         self.libvirt_domain.shutdown()

rpm_spec/core-dom0.spec

@@ -338,6 +338,7 @@ fi
 %attr(2770,root,qubes) %dir /var/lib/qubes/dvmdata
 %attr(2770,root,qubes) %dir /var/lib/qubes/vm-kernels
 /usr/share/qubes/templates/libvirt/xen.xml
+/usr/share/qubes/templates/libvirt/devices/pci.xml
 /usr/lib/tmpfiles.d/qubes.conf
 /usr/lib/qubes/qubes-prepare-saved-domain.sh
 /usr/lib/qubes/qubes-update-dispvm-savefile-with-progress.sh

templates/libvirt/devices/pci.xml

@@ -0,0 +1,10 @@
+<hostdev type="pci" managed="yes">
+    <source>
+        <address
+            bus="0x{{ device.bus }}"
+            slot="0x{{ device.device }}"
+            function="0x{{ device.function }}" />
+    </source>
+</hostdev>
+
+{# vim : set ft=jinja ts=4 sts=4 sw=4 et : #}

templates/libvirt/xen.xml

@@ -104,14 +104,7 @@
         {% endif %}
 
         {% for device in vm.devices.pci %}
-            <hostdev type="pci" managed="yes">
+            {% include libvirt/devices/pci.xml %}
-                <source>
-                    <address
-                        bus="0x{{ device.bus }}"
-                        slot="0x{{ device.device }}"
-                        function="0x{{ device.function }}" />
-                </source>
-            </hostdev>
         {% endfor %}
 
         {% if vm.hvm %}