From 361550c621762e1c89defe45d575b49bb520e7ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 29 Oct 2019 03:13:49 +0100 Subject: [PATCH 1/7] vm: improve error message about missing IOMMU Handle this case specifically, as way too many users ignore the message during installation and complain it doesn't work later. Name the problem explicitly, instead of pointing at libvirt error log. Fixes QubesOS/qubes-issues#4689 --- qubes/app.py | 10 ++++++++++ qubes/vm/qubesvm.py | 13 +++++++++++++ 2 files changed, 23 insertions(+) diff --git a/qubes/app.py b/qubes/app.py index 016a3cd9..8b7c9398 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -314,6 +314,16 @@ class QubesHost: raise NotImplementedError('This function requires Xen hypervisor') return int(self._physinfo['free_memory']) + def is_iommu_supported(self): + """Check if IOMMU is supported on this platform""" + if self._physinfo is None: + try: + self._physinfo = self.app.vmm.xc.physinfo() + except AttributeError: + raise NotImplementedError( + 'This function requires Xen hypervisor') + return 'hvm_directio' in self._physinfo['virt_caps'] + def get_vm_stats(self, previous_time=None, previous=None, only_vm=None): """Measure cpu usage for all domains at once. diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 2aea4330..9df7190f 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -1115,6 +1115,19 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.libvirt_domain.createWithFlags( libvirt.VIR_DOMAIN_START_PAUSED) + except libvirt.libvirtError as exc: + # missing IOMMU? + if self.virt_mode == 'hvm' and \ + list(self.devices['pci'].persistent()) and \ + not self.app.host.is_iommu_supported(): + exc = qubes.exc.QubesException( + 'Failed to start an HVM qube with PCI devices assigned ' + '- hardware does not support IOMMU/VT-d/AMD-Vi') + self.log.error('Start failed: %s', str(exc)) + yield from self.fire_event_async('domain-start-failed', + reason=str(exc)) + yield from self.storage.stop() + raise exc except Exception as exc: self.log.error('Start failed: %s', str(exc)) # let anyone receiving domain-pre-start know that startup failed From 5908ab1568055601130f57a9b47706952a3a1214 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 30 Oct 2019 15:46:11 +0100 Subject: [PATCH 2/7] app: fix get_free_xen_memory function It's app.vmm.xc, not app.xc. Since nobody have noticed it, this function might be unused... --- qubes/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/app.py b/qubes/app.py index 8b7c9398..1a3f2aed 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -309,7 +309,7 @@ class QubesHost: :raises NotImplementedError: when not under Xen """ try: - self._physinfo = self.app.xc.physinfo() + self._physinfo = self.app.vmm.xc.physinfo() except AttributeError: raise NotImplementedError('This function requires Xen hypervisor') return int(self._physinfo['free_memory']) From 598d059c57cd4fce89e3f4f9ce3e3f5541e52910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 31 Oct 2019 01:17:26 +0100 Subject: [PATCH 3/7] tests: check if storage driver adjust the size on import_volume/clone Regression test for QubesOS/qubes-issues#4821 --- qubes/tests/integ/storage.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/qubes/tests/integ/storage.py b/qubes/tests/integ/storage.py index 64441c54..526e5396 100644 --- a/qubes/tests/integ/storage.py +++ b/qubes/tests/integ/storage.py @@ -302,6 +302,35 @@ class StorageTestMixin(object): 'head -c {} /dev/zero 2>&1 | diff -q /dev/xvde - 2>&1'.format(size), user='root') + def test_005_size_after_clone(self): + '''Test snapshot volume non-persistence''' + return self.loop.run_until_complete( + self._test_005_size_after_clone()) + + @asyncio.coroutine + def _test_005_size_after_clone(self): + size = 128 * 1024 * 1024 + volume_config = { + 'pool': self.pool.name, + 'size': size, + 'save_on_stop': True, + 'rw': True, + } + testvol = self.vm1.storage.init_volume('testvol', volume_config) + yield from qubes.utils.coro_maybe(testvol.create()) + self.assertEquals(testvol.size, size) + volume_config = { + 'pool': self.pool.name, + 'size': size // 2, + 'save_on_stop': True, + 'rw': True, + } + testvol2 = self.vm2.storage.init_volume('testvol2', volume_config) + yield from qubes.utils.coro_maybe(testvol2.create()) + self.assertEquals(testvol2.size, size // 2) + yield from qubes.utils.coro_maybe(testvol2.import_volume(testvol)) + self.assertEquals(testvol2.size, size) + class StorageFile(StorageTestMixin, qubes.tests.SystemTestCase): def init_pool(self): From dd037f4663a55612dce429047086233a30a9c4e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 31 Oct 2019 01:37:06 +0100 Subject: [PATCH 4/7] storage/file: get volume size from the actual image file size Don't realy on a volume configuration only, it's easy to miss updating it. Specifically, import_volume() function didn't updated the size based on the source volume. The size that the actual VM sees is based on the file size, and so is the filesystem inside. Outdated size property can lead to a data loss if the user perform an action based on a incorrect assumption - like extending size, which actually will shrink the volume. Fixes QubesOS/qubes-issues#4821 --- qubes/storage/file.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/qubes/storage/file.py b/qubes/storage/file.py index b768fa51..7b12518e 100644 --- a/qubes/storage/file.py +++ b/qubes/storage/file.py @@ -28,6 +28,7 @@ import os import os.path import re import subprocess +from contextlib import suppress import qubes.storage @@ -394,6 +395,17 @@ class FileVolume(qubes.storage.Volume): iso_date = qubes.storage.isodate(seconds).split('.', 1)[0] return {'old': iso_date} + @property + def size(self): + with suppress(FileNotFoundError): + self._size = os.path.getsize(self.path) + return self._size + + @size.setter + def size(self, _): + raise qubes.storage.StoragePoolException( + "You shouldn't use volume size setter, use resize method instead") + @property def usage(self): ''' Returns the actualy used space ''' From 263f218d40fb93b97efacacc75808183f5e4dfb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sat, 9 Nov 2019 18:35:49 +0100 Subject: [PATCH 5/7] api/admin: implement admin.pool.volume.List method Similar to admin.vm.volume.List. There are still other admin.pool.volume.* methods missing, but lets start with just this one. Those with both pool name and volume id arguments may need some more thoughts. --- qubes/api/admin.py | 12 ++++++++++++ qubes/tests/api_admin.py | 14 ++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/qubes/api/admin.py b/qubes/api/admin.py index f9a9c35c..59f6354c 100644 --- a/qubes/api/admin.py +++ b/qubes/api/admin.py @@ -713,6 +713,18 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI): yield from self.app.remove_pool(self.arg) self.app.save() + @qubes.api.method('admin.pool.volume.List', no_payload=True, + scope='global', read=True) + @asyncio.coroutine + def pool_volume_list(self): + self.enforce(self.dest.name == 'dom0') + self.enforce(self.arg in self.app.pools.keys()) + + pool = self.app.pools[self.arg] + + volume_names = self.fire_event_for_filter(pool.volumes.keys()) + return ''.join('{}\n'.format(name) for name in volume_names) + @qubes.api.method('admin.pool.Set.revisions_to_keep', scope='global', write=True) @asyncio.coroutine diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index 47153ce6..fd9a91a8 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -2545,6 +2545,18 @@ class TC_00_VMs(AdminAPITestCase): with self.assertRaises(qubes.exc.QubesVMNotRunningError): self.call_mgmt_func(b'admin.vm.Console', b'test-vm1') + def test_700_pool_volume_list(self): + self.app.pools = { + 'pool1': unittest.mock.Mock(config={ + 'param1': 'value1', 'param2': 'value2'}, + usage=102400, + size=204800, + volumes={'vol1': unittest.mock.Mock(), + 'vol2': unittest.mock.Mock()}) + } + value = self.call_mgmt_func(b'admin.pool.volume.List', b'dom0', b'pool1') + self.assertEqual(value, 'vol1\nvol2\n') + def test_990_vm_unexpected_payload(self): methods_with_no_payload = [ b'admin.vm.List', @@ -2652,6 +2664,7 @@ class TC_00_VMs(AdminAPITestCase): b'admin.deviceclass.List', b'admin.vmclass.List', b'admin.vm.List', + b'admin.pool.volume.List', b'admin.label.List', b'admin.label.Get', b'admin.label.Remove', @@ -2735,6 +2748,7 @@ class TC_00_VMs(AdminAPITestCase): b'admin.label.Create', b'admin.label.Get', b'admin.label.Remove', + b'admin.pool.volume.List', b'admin.property.List', b'admin.property.Get', b'admin.property.Set', From 77cf310c47b67dd4d61e448e70ccebe21871ed10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 10 Nov 2019 00:34:33 +0100 Subject: [PATCH 6/7] storage/kernels: fix listing volumes Pool.volumes property is implemented in a base class, individual drivers should provide list_volumes() method as a backend for that property. Fix this in a LinuxKernel pool. --- qubes/storage/kernels.py | 3 +-- qubes/tests/storage_kernels.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py index 448057b1..b12d8663 100644 --- a/qubes/storage/kernels.py +++ b/qubes/storage/kernels.py @@ -209,8 +209,7 @@ class LinuxKernel(Pool): [pool for pool in app.pools.values() if pool is not self], self.dir_path) - @property - def volumes(self): + def list_volumes(self): ''' Return all known kernel volumes ''' return [LinuxModules(self.dir_path, kernel_version, diff --git a/qubes/tests/storage_kernels.py b/qubes/tests/storage_kernels.py index 1c035fb1..9b7fa5ff 100644 --- a/qubes/tests/storage_kernels.py +++ b/qubes/tests/storage_kernels.py @@ -250,7 +250,7 @@ class TC_03_KernelPool(qubes.tests.QubesTestCase): def test_002_pool_volumes(self): """ List volumes """ - volumes = self.app.pools[self.POOL_NAME].volumes + volumes = list(self.app.pools[self.POOL_NAME].volumes) self.assertEqual(len(volumes), 1) vol = volumes[0] self.assertEqual(vol.vid, 'dummy') From 7de9f3e078137843f5de6bf865d5c09a151a165c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 12 Nov 2019 22:45:30 +0100 Subject: [PATCH 7/7] tests: is_iommu_supported function QubesOS/qubes-issues#4689 --- qubes/tests/app.py | 50 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/qubes/tests/app.py b/qubes/tests/app.py index 0f06b641..9fec1ef5 100644 --- a/qubes/tests/app.py +++ b/qubes/tests/app.py @@ -153,6 +153,56 @@ class TC_20_QubesHost(qubes.tests.QubesTestCase): ('xc.domain_getinfo', (1, 1), {}), ]) + def test_010_iommu_supported(self): + self.app.vmm.configure_mock(**{ + 'xc.physinfo.return_value': { + 'hw_caps': '...', + 'scrub_memory': 0, + 'virt_caps': 'hvm hvm_directio', + 'nr_cpus': 4, + 'threads_per_core': 1, + 'cpu_khz': 3400001, + 'nr_nodes': 1, + 'free_memory': 234752, + 'cores_per_socket': 4, + 'total_memory': 16609720 + } + }) + self.assertEqual(self.qubes_host.is_iommu_supported(), True) + + def test_011_iommu_supported(self): + self.app.vmm.configure_mock(**{ + 'xc.physinfo.return_value': { + 'hw_caps': '...', + 'scrub_memory': 0, + 'virt_caps': 'hvm hvm_directio pv pv_directio', + 'nr_cpus': 4, + 'threads_per_core': 1, + 'cpu_khz': 3400001, + 'nr_nodes': 1, + 'free_memory': 234752, + 'cores_per_socket': 4, + 'total_memory': 16609720 + } + }) + self.assertEqual(self.qubes_host.is_iommu_supported(), True) + + def test_010_iommu_supported(self): + self.app.vmm.configure_mock(**{ + 'xc.physinfo.return_value': { + 'hw_caps': '...', + 'scrub_memory': 0, + 'virt_caps': 'hvm pv', + 'nr_cpus': 4, + 'threads_per_core': 1, + 'cpu_khz': 3400001, + 'nr_nodes': 1, + 'free_memory': 234752, + 'cores_per_socket': 4, + 'total_memory': 16609720 + } + }) + self.assertEqual(self.qubes_host.is_iommu_supported(), False) class TC_30_VMCollection(qubes.tests.QubesTestCase):