[Kimchi-devel] [PATCH] [Kimchi] Bug fix #1096 - Prevent mass detach based on source PCI address
Daniel Henrique Barboza
danielhb at linux.vnet.ibm.com
Thu Jan 26 12:37:03 UTC 2017
On 01/26/2017 10:23 AM, Aline Manera wrote:
>
>
> On 01/25/2017 05:52 PM, 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})
>> +
>
> I think when you raise an exception inside a AsyncTask it will be
> automatically caught on AsynTask.__exit__() and raise forewords so you
> don't need to do both things above.
I didn't pay much attention to it because it was existing code that I
just moved up.
You're saying that the 'raise NotFoundError' is ambiguous in this code and
shouldn't be used?
I'll fix it in v2 possibly with more logic changes as well.
Daniel
>
>> + 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):
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
More information about the Kimchi-devel
mailing list