[Kimchi-devel] [PATCH] [Kimchi] Bug fix #1096 - Prevent mass detach based on source PCI address
Lucio Correia
luciojhc at linux.vnet.ibm.com
Thu Jan 26 12:15:39 UTC 2017
Reviewed-By: Lucio Correia <luciojhc at linux.vnet.ibm.com>
On 25/01/2017 17:52, dhbarboza82 at gmail.com wrote:
> From: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
>
> Before this commit, Kimchi was mass detaching PCI devices when
> trying to detach a single dev 'hostdev0' following this criteria:
>
> - using the source address element, see all hostdevs that share
> the same PCI address (domain x bus x slot) as 'hostdev0';
>
> - if more than one is found, 'hostdev0' is considered to be
> a multifunction device an a mass detachment of all devices found
> is made.
>
> This logic is flawed when considering SR-IOV devices (CX4 cards
> for example). Although all virtual functions share the same
> source PCI address, each VF should be attached and detached
> individually.
>
> Following the current libvirt documentation
> (https://libvirt.org/formatdomain.html), there is an attribute
> called 'multifunction' that can be used:
>
> 'Also available is the multifunction attribute, which controls
> turning on the multifunction bit for a particular slot/function
> in the PCI control register (...) multifunction defaults to 'off',
> but should be set to 'on' for function 0 of a slot that will have
> multiple functions used.'
>
> Further experimentation showed that, in multifunction devices,
> the device PCI address differs only in the 'function' attribute,
> while in the VFs for example they are different. This means that
> the device address is a better place to look for paired devices
> instead of using the source PCI address.
>
> This patch implements these changes to fix #1096: use the multifunction
> attribute to determine if a hostdev is multifuction and changed
> the 'get_devices_same_addr' method to use the device address instead
> of the source PCI address as comparison.
>
> Minor simplications of vmhostdevs.py code were also made, such
> as use a dict instead of an array of tuples to easily retrieve
> elements by a key and moved the NotFound verification up in
> the '_detach_device' method.
>
> Unit tests included.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
> ---
> model/vmhostdevs.py | 112 +++++++++++++++++++++--------------
> tests/test_model.py | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 234 insertions(+), 42 deletions(-)
>
> diff --git a/model/vmhostdevs.py b/model/vmhostdevs.py
> index 9823537..45939b8 100644
> --- a/model/vmhostdevs.py
> +++ b/model/vmhostdevs.py
> @@ -635,7 +635,7 @@ class VMHostDevModel(object):
> wok_log.error('opaque must be valid')
> return
>
> - wok_log.info("Device %s removed successfuly" % alias)
> + wok_log.info("Device %s removed successfully" % alias)
> # Re-attach device to host
> try:
> dev = conn.get().nodeDeviceLookupByName(alias)
> @@ -655,8 +655,8 @@ class VMHostDevModel(object):
> lock = params['lock']
>
> with lock:
> - pci_devs = [(DeviceModel.deduce_dev_name(e, self.conn), e)
> - for e in hostdev if e.attrib['type'] == 'pci']
> + pci_devs = {DeviceModel.deduce_dev_name(e, self.conn): e
> + for e in hostdev if e.attrib['type'] == 'pci'}
>
> dev_info = self.dev_model.lookup(dev_name)
> is_3D_device = self.dev_model.is_device_3D_controller(dev_info)
> @@ -664,9 +664,20 @@ class VMHostDevModel(object):
> raise InvalidOperation('KCHVMHDEV0006E',
> {'name': dev_info['name']})
>
> + if not pci_devs.get(dev_name):
> + msg = WokMessage('KCHVMHDEV0001E',
> + {'vmid': vmid, 'dev_name': dev_name})
> + cb(msg.get_text(), False)
> + raise NotFoundError('KCHVMHDEV0001E',
> + {'vmid': vmid, 'dev_name': dev_name})
> +
> + dev_name_elem = pci_devs[dev_name]
> +
> # check for multifunction and detach all functions together
> try:
> - multi = self._unplug_multifunction_pci(dom, hostdev, dev_name)
> + multi = self.unplug_multifunction_pci(
> + dom, hostdev, dev_name_elem
> + )
> except libvirt.libvirtError:
> multi = False
>
> @@ -680,54 +691,71 @@ class VMHostDevModel(object):
> cb('OK', True)
> return
>
> - # detach each function individually
> - for e in hostdev:
> - if DeviceModel.deduce_dev_name(e, self.conn) == dev_name:
> - xmlstr = etree.tostring(e)
> - dom.detachDeviceFlags(
> - xmlstr, get_vm_config_flag(dom, mode='all'))
> - if e.attrib['type'] == 'pci':
> - self._delete_affected_pci_devices(dom, dev_name,
> - pci_devs)
> - if is_3D_device:
> - devsmodel = VMHostDevsModel(conn=self.conn)
> - devsmodel.update_mmio_guest(vmid, False)
> - break
> - else:
> - msg = WokMessage('KCHVMHDEV0001E',
> - {'vmid': vmid, 'dev_name': dev_name})
> - cb(msg.get_text(), False)
> - raise NotFoundError('KCHVMHDEV0001E',
> - {'vmid': vmid, 'dev_name': dev_name})
> + # detach individually
> + xmlstr = etree.tostring(dev_name_elem)
> + dom.detachDeviceFlags(
> + xmlstr, get_vm_config_flag(dom, mode='all'))
> + if dev_name_elem.attrib['type'] == 'pci':
> + self._delete_affected_pci_devices(dom, dev_name,
> + pci_devs)
> + if is_3D_device:
> + devsmodel = VMHostDevsModel(conn=self.conn)
> + devsmodel.update_mmio_guest(vmid, False)
>
> if DOM_STATE_MAP[dom.info()[0]] == "shutoff":
> cb('OK', True)
>
> - def _get_devices_same_addr(self, hostdev, domain, bus, slot):
> + def get_devices_same_addr(self, hostdevs, device_elem):
> + def elem_has_valid_address(elem):
> + if elem.get('type') != 'pci' or \
> + elem.address is None or \
> + elem.address.get('domain') is None or \
> + elem.address.get('bus') is None or \
> + elem.address.get('slot') is None:
> + return False
> + return True
> +
> + if not elem_has_valid_address(device_elem):
> + return []
> +
> devices = []
> - for device in hostdev:
> - if device.attrib['type'] != 'pci':
> - continue
> + device_domain = device_elem.address.get('domain')
> + device_bus = device_elem.address.get('bus')
> + device_slot = device_elem.address.get('slot')
>
> - address = device.source.address
> - if int(address.attrib['domain'], 16) != domain or \
> - int(address.attrib['bus'], 16) != bus or \
> - int(address.attrib['slot'], 16) != slot:
> + for dev in hostdevs:
> +
> + if not elem_has_valid_address(dev):
> continue
>
> - devices.append(etree.tostring(device))
> + dev_domain = dev.address.get('domain')
> + dev_bus = dev.address.get('bus')
> + dev_slot = dev.address.get('slot')
> +
> + if dev_domain == device_domain and dev_bus == device_bus and \
> + dev_slot == device_slot:
> + devices.append(etree.tostring(dev))
>
> return devices
>
> - def _unplug_multifunction_pci(self, dom, hostdev, dev_name):
> - # get all devices attached to the guest in the same domain+bus+slot
> - # that the one we are going to detach because they must be detached
> - # together
> - domain, bus, slot, _ = dev_name.split('_')[1:]
> - devices = self._get_devices_same_addr(hostdev,
> - int(domain, 16),
> - int(bus, 16),
> - int(slot, 16))
> + def is_hostdev_multifunction(self, dev_elem):
> + if dev_elem.address is None or \
> + dev_elem.address.get('multifunction') is None or \
> + dev_elem.address.get('function') is None:
> +
> + return False
> +
> + is_multi = dev_elem.address.get('multifunction') == 'on' and \
> + dev_elem.address.get('function') == '0x0'
> +
> + return is_multi
> +
> + def unplug_multifunction_pci(self, dom, hostdevs, dev_elem):
> + if not self.is_hostdev_multifunction(dev_elem):
> + return False
> +
> + devices = self.get_devices_same_addr(hostdevs, dev_elem)
> +
> if len(devices) <= 1:
> return False
>
> @@ -747,7 +775,7 @@ class VMHostDevModel(object):
> DevicesModel(
> conn=self.conn).get_list(_passthrough_affected_by=dev_name))
>
> - for pci_name, e in pci_devs:
> + for pci_name, e in pci_devs.iteritems():
> if pci_name in affected_names:
> xmlstr = etree.tostring(e)
> dom.detachDeviceFlags(
> diff --git a/tests/test_model.py b/tests/test_model.py
> index 0209e8e..a14cf56 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -34,6 +34,8 @@ import shutil
> import time
> import unittest
>
> +from lxml import objectify
> +
> import tests.utils as utils
>
> import wok.objectstore
> @@ -1783,6 +1785,168 @@ class ModelTests(unittest.TestCase):
> self.assertTrue(
> inst.vmhostdevs_have_usb_controller('kimchi-vm1'))
>
> + def get_hostdevs_xml(self):
> + return """\
> +<domain type='kvm' id='N'>
> + <name>vm_name</name>
> + <devices>
> + <emulator>/usr/bin/qemu-kvm</emulator>
> + <hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x1'/>
> + </source>
> + <alias name='hostdev0'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' \
> +function='0x1'/>
> + </hostdev>
> + <hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x0'/>
> + </source>
> + <alias name='hostdev1'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' \
> +function='0x0' multifunction='on'/>
> + </hostdev>
> + <hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x2'/>
> + </source>
> + <alias name='hostdev2'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' \
> +function='0x0'/>
> + </hostdev>
> + <hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x4'/>
> + </source>
> + <alias name='hostdev3'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' \
> +function='0x0'/>
> + </hostdev>
> + <hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x5'/>
> + </source>
> + <alias name='hostdev4'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' \
> +function='0x0'/>
> + </hostdev>
> + </devices>
> + </domain>
> +"""
> +
> + def get_hostdev_multifunction_xml(self):
> + return """\
> +<hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x0'/>
> + </source>
> + <alias name='hostdev1'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0' \
> +multifunction='on'/>
> +</hostdev>
> +"""
> +
> + def get_hostdev_nomultifunction_xml(self):
> + return """\
> +<hostdev mode='subsystem' type='pci' managed='yes'>
> + <driver name='vfio'/>
> + <source>
> + <address domain='0x0001' bus='0x0d' slot='0x00' function='0x5'/>
> + </source>
> + <alias name='hostdev4'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
> +</hostdev>
> +"""
> +
> + def test_vmhostdev_is_hostdev_multifunction(self):
> + inst = model.Model(None, objstore_loc=self.tmp_store)
> +
> + hostdev_multi_elem = objectify.fromstring(
> + self.get_hostdev_multifunction_xml()
> + )
> + self.assertTrue(
> + inst.vmhostdev_is_hostdev_multifunction(hostdev_multi_elem)
> + )
> +
> + hostdev_nomulti_elem = objectify.fromstring(
> + self.get_hostdev_nomultifunction_xml()
> + )
> + self.assertFalse(
> + inst.vmhostdev_is_hostdev_multifunction(hostdev_nomulti_elem)
> + )
> +
> + def test_vmhostdev_get_devices_same_addr(self):
> + inst = model.Model(None, objstore_loc=self.tmp_store)
> +
> + root = objectify.fromstring(self.get_hostdevs_xml())
> + hostdevs = root.devices.hostdev
> +
> + hostdev_multi_elem = objectify.fromstring(
> + self.get_hostdev_multifunction_xml()
> + )
> +
> + hostdev_same_addr_str = """\
> +<hostdev mode="subsystem" type="pci" managed="yes"><driver name="vfio"/>\
> +<source><address domain="0x0001" bus="0x0d" slot="0x00" function="0x1"/>\
> +</source><alias name="hostdev0"/>\
> +<address type="pci" domain="0x0000" bus="0x00" slot="0x05" function="0x1"/>\
> +</hostdev>"""
> + same_addr_devices = [
> + ET.tostring(hostdev_multi_elem), hostdev_same_addr_str
> + ]
> +
> + self.assertItemsEqual(
> + same_addr_devices,
> + inst.vmhostdev_get_devices_same_addr(hostdevs, hostdev_multi_elem)
> + )
> +
> + nomatch_elem = objectify.fromstring(
> + self.get_hostdev_nomultifunction_xml()
> + )
> +
> + self.assertEqual(
> + inst.vmhostdev_get_devices_same_addr(hostdevs, nomatch_elem),
> + [ET.tostring(nomatch_elem)]
> + )
> +
> + @mock.patch('wok.plugins.kimchi.model.vmhostdevs.get_vm_config_flag')
> + def test_vmhostdev_unplug_multifunction_pci(self, mock_conf_flag):
> + class FakeDom():
> + def detachDeviceFlags(self, xml, config_flag):
> + pass
> +
> + mock_conf_flag.return_value = ''
> +
> + inst = model.Model(None, objstore_loc=self.tmp_store)
> +
> + root = objectify.fromstring(self.get_hostdevs_xml())
> + hostdevs = root.devices.hostdev
> +
> + hostdev_multi_elem = objectify.fromstring(
> + self.get_hostdev_multifunction_xml()
> + )
> +
> + self.assertTrue(
> + inst.vmhostdev_unplug_multifunction_pci(FakeDom(), hostdevs,
> + hostdev_multi_elem)
> + )
> +
> + nomatch_elem = objectify.fromstring(
> + self.get_hostdev_nomultifunction_xml()
> + )
> +
> + self.assertFalse(
> + inst.vmhostdev_unplug_multifunction_pci(FakeDom(), hostdevs,
> + nomatch_elem)
> + )
> +
>
> class BaseModelTests(unittest.TestCase):
> class FoosModel(object):
>
--
Lucio Correia
More information about the Kimchi-devel
mailing list