From 4dc863101067ee66fa13c3e5d2c8e20d0d80eac8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sat, 3 Nov 2018 05:13:23 +0100 Subject: [PATCH 1/4] Use maxmem=0 to disable qmemman, add more automation to it Use maxmem=0 for disabling dynamic memory balance, instead of cryptic service.meminfo-writer feature. Under the hood, meminfo-writer service is also set based on maxmem property (directly in qubesdb, not vm.features dict). Having this as a property (not "feature"), allow to have sensible handling of default value. Specifically, disable it automatically if otherwise it would crash a VM. This is the case for: - domain with PCI devices (PoD is not supported by Xen then) - domain without balloon driver and/or meminfo-writer service The check for the latter is heuristic (assume presence of 'qrexec' also can indicate balloon driver support), but it is true for currently supported systems. This also allows more reliable control of libvirt config: do not set memory != maxmem, unless qmemman is enabled. memory != maxmem only makes sense if qmemman for given domain is enabled. Besides wasting some domain resources for extra page tables etc, for HVM domains this is harmful, because maxmem-memory difference is made of Popupate-on-Demand pool, which - when depleted - will kill the domain. This means domain without balloon driver will die as soon as will try to use more than initial memory - but without balloon driver it sees maxmem memory and doesn't know about the lower limit. Fixes QubesOS/qubes-issues#4135 --- qubes/ext/services.py | 4 +++ qubes/tests/ext.py | 2 ++ qubes/tests/vm/qubesvm.py | 6 +++- qubes/vm/qubesvm.py | 72 +++++++++++++++++++++++++++++++++------ templates/libvirt/xen.xml | 7 ++-- 5 files changed, 76 insertions(+), 15 deletions(-) diff --git a/qubes/ext/services.py b/qubes/ext/services.py index b09e2fda..cb9cd5f7 100644 --- a/qubes/ext/services.py +++ b/qubes/ext/services.py @@ -39,6 +39,10 @@ class ServicesExtension(qubes.ext.Extension): vm.untrusted_qdb.write('/qubes-service/{}'.format(service), str(int(bool(value)))) + # always set meminfo-writer according to maxmem + vm.untrusted_qdb.write('/qubes-service/meminfo-writer', + '1' if vm.maxmem > 0 else '0') + @qubes.ext.handler('domain-feature-set:*') def on_domain_feature_set(self, vm, event, feature, value, oldvalue=None): '''Update /qubes-service/ QubesDB tree in runtime''' diff --git a/qubes/tests/ext.py b/qubes/tests/ext.py index 6f59132f..094c80ca 100644 --- a/qubes/tests/ext.py +++ b/qubes/tests/ext.py @@ -224,6 +224,7 @@ class TC_20_Services(qubes.tests.QubesTestCase): self.features = {} self.vm.configure_mock(**{ 'template': None, + 'maxmem': 1024, 'is_running.return_value': True, 'features.get.side_effect': self.features.get, 'features.items.side_effect': self.features.items, @@ -239,6 +240,7 @@ class TC_20_Services(qubes.tests.QubesTestCase): self.ext.on_domain_qdb_create(self.vm, 'domain-qdb-create') self.assertEqual(sorted(self.vm.untrusted_qdb.mock_calls), [ + ('write', ('/qubes-service/meminfo-writer', '1'), {}), ('write', ('/qubes-service/test1', '1'), {}), ('write', ('/qubes-service/test2', '0'), {}), ]) diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py index 308fd782..cb4df88d 100644 --- a/qubes/tests/vm/qubesvm.py +++ b/qubes/tests/vm/qubesvm.py @@ -665,7 +665,7 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): expected = ''' test-inst-test 7db78950-c467-4863-94d1-af59806384ea - 500 + 400 400 2 @@ -766,6 +766,7 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): vm = self.get_vm(uuid=my_uuid) vm.netvm = None vm.virt_mode = 'hvm' + vm.features['qrexec'] = True with unittest.mock.patch('qubes.config.qubes_base_dir', '/tmp/qubes-test'): kernel_dir = '/tmp/qubes-test/vm-kernels/dummy' @@ -906,6 +907,7 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): vm = self.get_vm(uuid=my_uuid) vm.netvm = netvm vm.virt_mode = 'hvm' + vm.features['qrexec'] = True with self.subTest('ipv4_only'): libvirt_xml = vm.create_config_file() self.assertXMLEqual(lxml.etree.XML(libvirt_xml), @@ -969,6 +971,7 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): '/qubes-iptables-error': '', '/qubes-iptables-header': iptables_header, '/qubes-service/qubes-update-check': '0', + '/qubes-service/meminfo-writer': '1', }) @unittest.mock.patch('qubes.utils.get_timezone') @@ -1029,6 +1032,7 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): '/qubes-iptables-error': '', '/qubes-iptables-header': iptables_header, '/qubes-service/qubes-update-check': '0', + '/qubes-service/meminfo-writer': '1', '/qubes-ip': '10.137.0.3', '/qubes-netmask': '255.255.255.255', '/qubes-gateway': '10.137.0.2', diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 2b46c5b6..6b7abad9 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -69,13 +69,21 @@ def _setter_kernel(self, prop, value): def _setter_positive_int(self, prop, value): - ''' Helper for setting a positive int. Checks that the int is >= 0 ''' + ''' Helper for setting a positive int. Checks that the int is > 0 ''' # pylint: disable=unused-argument value = int(value) if value <= 0: raise ValueError('Value must be positive') return value +def _setter_non_negative_int(self, prop, value): + ''' Helper for setting a positive int. Checks that the int is >= 0 ''' + # pylint: disable=unused-argument + value = int(value) + if value < 0: + raise ValueError('Value must be positive or zero') + return value + def _setter_default_user(self, prop, value): ''' Helper for setting default user ''' @@ -122,6 +130,25 @@ def _default_with_template(prop, default): return _func +def _default_maxmem(self): + # first check for any reason to _not_ enable qmemman + if not self.is_memory_balancing_possible(): + return 0 + + # Linux specific cap: max memory can't scale beyond 10.79*init_mem + # see https://groups.google.com/forum/#!topic/qubes-devel/VRqkFj1IOtA + if self.features.get('os', None) == 'Linux': + default_maxmem = self.memory * 10 + else: + default_maxmem = 4000 + + # don't use default larger than half of physical ram + default_maxmem = min(default_maxmem, + int(self.app.host.memory_total / 1024 / 2)) + + return _default_with_template('maxmem', default_maxmem)(self) + + class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): '''Base functionality of Qubes VM shared between all VMs. @@ -448,12 +475,12 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): 'template\'s value by default.') maxmem = qubes.property('maxmem', type=int, - setter=_setter_positive_int, - default=_default_with_template('maxmem', (lambda self: - int(min(self.app.host.memory_total / 1024 / 2, 4000)))), + setter=_setter_non_negative_int, + default=_default_maxmem, doc='''Maximum amount of memory available for this VM (for the purpose - of the memory balancer). TemplateBasedVMs use its ' - 'template\'s value by default.''') + of the memory balancer). Set to 0 to disable memory balancing for + this qube. TemplateBasedVMs use its template\'s value by default + (unless memory balancing not supported for this qube).''') stubdom_mem = qubes.property('stubdom_mem', type=int, setter=_setter_positive_int, @@ -743,11 +770,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): assert hasattr(self, 'qid') assert hasattr(self, 'name') - # Linux specific cap: max memory can't scale beyond 10.79*init_mem - # see https://groups.google.com/forum/#!topic/qubes-devel/VRqkFj1IOtA - if self.maxmem > self.memory * 10: - self.maxmem = self.memory * 10 - if xml is None: # new qube, disable updates check if requested for new qubes # SEE: 1637 when features are done, migrate to plugin @@ -1350,6 +1372,34 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): return stdouterr + def is_memory_balancing_possible(self): + '''Check if memory balancing can be enabled. + Reasons to not enable it: + - have PCI devices + - balloon driver not present + + We don't have reliable way to detect the second point, but good + heuristic is HVM virt_mode (PV and PVH require OS support and it does + include balloon driver) and lack of qrexec/meminfo-writer service + support (no qubes tools installed). + ''' + if list(self.devices['pci'].persistent()): + return False + if self.virt_mode == 'hvm': + # if VM announce any supported service + features_set = set(self.features) + template = getattr(self, 'template', None) + while template is not None: + features_set.update(template.features) + template = getattr(template, 'template', None) + supported_services = any(f.startswith('supported-service.') + for f in features_set) + if (not self.features.check_with_template('qrexec', False) or + (supported_services and not self.features.check_with_template( + 'supported-service.meminfo-writer', False))): + return False + return True + def request_memory(self, mem_required=None): # overhead of per-qube/per-vcpu Xen structures, # taken from OpenStack nova/virt/xenapi/driver.py diff --git a/templates/libvirt/xen.xml b/templates/libvirt/xen.xml index 2829b0f9..3e6cf752 100644 --- a/templates/libvirt/xen.xml +++ b/templates/libvirt/xen.xml @@ -2,11 +2,12 @@ {% block basic %} {{ vm.name }} {{ vm.uuid }} - {% if vm.virt_mode == 'hvm' and vm.devices['pci'].persistent() | list %} + {% if ((vm.virt_mode == 'hvm' and vm.devices['pci'].persistent() | list) + or vm.maxmem == 0) -%} {{ vm.memory }} - {% else %} + {% else -%} {{ vm.maxmem }} - {% endif %} + {% endif -%} {{ vm.memory }} {{ vm.vcpus }} {% endblock %} From b8052f864ab7c882e430e8eda5199874fedf6786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 15 Nov 2018 10:14:57 +0100 Subject: [PATCH 2/4] tests: more cases for libvirt xml generation Related to automatic mem balance enabling/disabling. Check how it behave in presence of PCI devices, or explicit disabling it. --- qubes/tests/vm/qubesvm.py | 149 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py index cb4df88d..8ce6b9a7 100644 --- a/qubes/tests/vm/qubesvm.py +++ b/qubes/tests/vm/qubesvm.py @@ -849,6 +849,155 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase): self.assertXMLEqual(lxml.etree.XML(libvirt_xml), lxml.etree.XML(expected)) + def test_600_libvirt_xml_pvh_no_membalance(self): + expected = ''' + test-inst-test + 7db78950-c467-4863-94d1-af59806384ea + 400 + 400 + 2 + + + + + + + + + pvh + /tmp/kernel/vmlinuz + /tmp/kernel/initramfs + root=/dev/mapper/dmroot ro nomodeset console=hvc0 rd_NO_PLYMOUTH rd.plymouth.enable=0 plymouth.enable=0 nopat + + + + + + + + + + + destroy + destroy + destroy + + + + + + + + + + + + + ''' + my_uuid = '7db78950-c467-4863-94d1-af59806384ea' + vm = self.get_vm(uuid=my_uuid) + vm.netvm = None + vm.virt_mode = 'pvh' + vm.maxmem = 0 + with unittest.mock.patch('qubes.config.qubes_base_dir', + '/tmp/qubes-test'): + kernel_dir = '/tmp/qubes-test/vm-kernels/dummy' + os.makedirs(kernel_dir, exist_ok=True) + open(os.path.join(kernel_dir, 'vmlinuz'), 'w').close() + open(os.path.join(kernel_dir, 'initramfs'), 'w').close() + self.addCleanup(shutil.rmtree, '/tmp/qubes-test') + vm.kernel = 'dummy' + # tests for storage are later + vm.volumes['kernel'] = unittest.mock.Mock(**{ + 'kernels_dir': '/tmp/kernel', + 'block_device.return_value.domain': 'dom0', + 'block_device.return_value.script': None, + 'block_device.return_value.path': '/tmp/kernel/modules.img', + 'block_device.return_value.devtype': 'disk', + 'block_device.return_value.name': 'kernel', + }) + libvirt_xml = vm.create_config_file() + self.assertXMLEqual(lxml.etree.XML(libvirt_xml), + lxml.etree.XML(expected)) + + def test_600_libvirt_xml_hvm_pcidev(self): + expected = ''' + test-inst-test + 7db78950-c467-4863-94d1-af59806384ea + 400 + 400 + 2 + + + + + + + + + hvm + + hvmloader + + + + + + + + + + + + + + destroy + destroy + destroy + + + +
+ + + + + + + + + + ''' + my_uuid = '7db78950-c467-4863-94d1-af59806384ea' + # required for PCI devices listing + self.app.vmm.offline_mode = False + vm = self.get_vm(uuid=my_uuid) + vm.netvm = None + vm.virt_mode = 'hvm' + vm.kernel = None + # even with meminfo-writer enabled, should have memory==maxmem + vm.features['service.meminfo-writer'] = True + assignment = qubes.devices.DeviceAssignment( + vm, # this is violation of API, but for PCI the argument + # is unused + '00_00.0', + bus='pci', + persistent=True) + vm.devices['pci']._set.add( + assignment) + libvirt_xml = vm.create_config_file() + self.assertXMLEqual(lxml.etree.XML(libvirt_xml), + lxml.etree.XML(expected)) + def test_610_libvirt_xml_network(self): expected = ''' test-inst-test From 62bc462a2349ec3deb93333c2c131642f2a43b4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 15 Nov 2018 13:43:58 +0100 Subject: [PATCH 3/4] tests: default maxmem --- qubes/tests/vm/qubesvm.py | 61 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py index 8ce6b9a7..558f7c50 100644 --- a/qubes/tests/vm/qubesvm.py +++ b/qubes/tests/vm/qubesvm.py @@ -44,6 +44,22 @@ class TestApp(object): def __init__(self): self.domains = {} + self.host = unittest.mock.Mock() + self.host.memory_total = 4096 * 1024 + +class TestFeatures(dict): + def __init__(self, vm, **kwargs) -> None: + self.vm = vm + super().__init__(**kwargs) + + def check_with_template(self, feature, default): + vm = self.vm + while vm is not None: + try: + return vm.features[feature] + except KeyError: + vm = getattr(vm, 'template', None) + return default class TestProp(object): # pylint: disable=too-few-public-methods @@ -80,6 +96,7 @@ class TestVM(object): for k, v in kwargs.items(): setattr(self, k, v) self.devices = {'pci': TestDeviceCollection()} + self.features = TestFeatures(self) def is_running(self): return self.running @@ -170,8 +187,6 @@ class TC_10_default(qubes.tests.QubesTestCase): self.assertEqual(default_getter(self.vm), 'template-kernel') def test_010_default_virt_mode(self): - default_getter = qubes.vm.qubesvm._default_with_template('kernel', - lambda x: x.app.default_kernel) self.assertEqual(qubes.vm.qubesvm._default_virt_mode(self.vm), 'pvh') self.vm.template = unittest.mock.Mock() @@ -185,6 +200,48 @@ class TC_10_default(qubes.tests.QubesTestCase): self.assertEqual(qubes.vm.qubesvm._default_virt_mode(self.vm), 'hvm') + def test_020_default_maxmem(self): + default_maxmem = 2048 + self.vm.is_memory_balancing_possible = \ + lambda: qubes.vm.qubesvm.QubesVM.is_memory_balancing_possible( + self.vm) + self.vm.virt_mode = 'pvh' + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), + default_maxmem) + self.vm.virt_mode = 'hvm' + # HVM without qubes tools + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), 0) + # just 'qrexec' feature + self.vm.features['qrexec'] = True + print(self.vm.features.check_with_template('qrexec', False)) + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), + default_maxmem) + # some supported-service.*, but not meminfo-writer + self.vm.features['supported-service.qubes-firewall'] = True + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), 0) + # then add meminfo-writer + self.vm.features['supported-service.meminfo-writer'] = True + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), + default_maxmem) + + def test_021_default_maxmem_with_pcidevs(self): + self.vm.is_memory_balancing_possible = \ + lambda: qubes.vm.qubesvm.QubesVM.is_memory_balancing_possible( + self.vm) + self.vm.devices['pci'].persistent().append('00_00.0') + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), 0) + + def test_022_default_maxmem_linux(self): + self.vm.is_memory_balancing_possible = \ + lambda: qubes.vm.qubesvm.QubesVM.is_memory_balancing_possible( + self.vm) + self.vm.virt_mode = 'pvh' + self.vm.memory = 400 + self.vm.features['os'] = 'Linux' + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), 2048) + self.vm.memory = 100 + self.assertEqual(qubes.vm.qubesvm._default_maxmem(self.vm), 1000) + class QubesVMTestsMixin(object): property_no_default = object() From 087a02c7f493dcb7da006d9d5fab204321b97c42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 15 Nov 2018 15:02:14 +0100 Subject: [PATCH 4/4] ext/services: add automatic migration meminfo-writer=False -> maxmem=0 Migrate meminfo-writer=False service setting to maxmem=0 as a method to disable dynamic memory management. Remove the service from vm.features dict in the process. Additionally, translate any attempt to set the service.meminfo-writer feature to either setting maxmem=0 or resetting it to the default (which is memory balancing enabled if supported by given domain). This is to at least partially not break existing tools using service.meminfo-writer as a way to control dynamic memory management. This code does _not_ support reading service.meminfo-writer feature state to get the current state of dynamic memory management, as it would require synchronizing with all the factors affecting its value. One of main reasons for migrating to maxmem=0 approach is to avoid the need of such synchronization. QubesOS/qubes-issues#4480 --- qubes/ext/services.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/qubes/ext/services.py b/qubes/ext/services.py index cb9cd5f7..52557d89 100644 --- a/qubes/ext/services.py +++ b/qubes/ext/services.py @@ -47,6 +47,20 @@ class ServicesExtension(qubes.ext.Extension): def on_domain_feature_set(self, vm, event, feature, value, oldvalue=None): '''Update /qubes-service/ QubesDB tree in runtime''' # pylint: disable=unused-argument + + # TODO: remove this compatibility hack in Qubes 4.1 + if feature == 'service.meminfo-writer': + # if someone try to enable meminfo-writer ... + if value: + # ... reset maxmem to default + vm.maxmem = qubes.property.DEFAULT + else: + # otherwise, set to 0 + vm.maxmem = 0 + # in any case, remove the entry, as it does not indicate memory + # balancing state anymore + del vm.features['service.meminfo-writer'] + if not vm.is_running(): return if not feature.startswith('service.'): @@ -65,8 +79,22 @@ class ServicesExtension(qubes.ext.Extension): if not feature.startswith('service.'): return service = feature[len('service.'):] + # this one is excluded from user control + if service == 'meminfo-writer': + return vm.untrusted_qdb.rm('/qubes-service/{}'.format(service)) + @qubes.ext.handler('domain-load') + def on_domain_load(self, vm, event): + '''Migrate meminfo-writer service into maxmem''' + # pylint: disable=no-self-use,unused-argument + if 'service.meminfo-writer' in vm.features: + # if was set to false, force maxmem=0 + # otherwise, simply ignore as the default is fine + if not vm.features['service.meminfo-writer']: + vm.maxmem = 0 + del vm.features['service.meminfo-writer'] + @qubes.ext.handler('features-request') def supported_services(self, vm, event, untrusted_features): '''Handle advertisement of supported services'''