Merge "libvirt: Use firmware auto-selection by libvirt"

nova/tests/fixtures/libvirt.py

@@ -17,7 +17,6 @@ import os
 import sys
 import textwrap
 import time
-from unittest import mock
 
 import fixtures
 from lxml import etree
@@ -1189,7 +1188,16 @@ class Domain(object):
 
         os_loader = tree.find('./os/loader')
         if os_loader is not None:
+            os['loader'] = os_loader.text
+            os['loader_type'] = os_loader.get('type')
+            os['loader_readonly'] = os_loader.get('readonly')
             os['loader_stateless'] = os_loader.get('stateless')
+            os['loader_secure'] = os_loader.get('secure')
+
+        os_nvram = tree.find('./os/nvram')
+        if os_nvram is not None:
+            os['nvram'] = os_nvram.text
+            os['nvram_template'] = os_nvram.get('template')
 
         os_kernel = tree.find('./os/kernel')
         if os_kernel is not None:
@@ -1562,10 +1570,35 @@ class Domain(object):
         pass
 
     def XMLDesc(self, flags):
-        loader = '<loader/>'
+        loader_elems = ['']
+        if self._def['os'].get('loader_type'):
+            loader_elems.append(
+                "type='%s'" % self._def['os'].get('loader_type'))
+        if self._def['os'].get('loader_readonly'):
+            loader_elems.append(
+                "readonly='%s'" % self._def['os'].get('loader_readonly'))
+        if self._def['os'].get('loader_secure'):
+            loader_elems.append(
+                "secure='%s'" % self._def['os'].get('loader_secure'))
         if self._def['os'].get('loader_stateless'):
-            loader = ('<loader stateless="%s"/>' %
+            loader_elems.append(
-                self._def['os'].get('loader_stateless'))
+                "stateless='%s'" % self._def['os'].get('loader_stateless'))
+
+        loader = ''
+        if self._def['os'].get('loader'):
+            loader = '<loader%s>%s</loader>' % (
+                ' '.join(loader_elems), self._def['os'].get('loader'))
+        elif loader_elems:
+            loader = '<loader%s/>' % ' '.join(loader_elems)
+
+        nvram = ''
+        nvram_template = self._def['os'].get('nvram_template')
+        if nvram_template:
+            if not self._def['os'].get('nvram'):
+                nvram = "<nvram template='%s'/>" % nvram_template
+            else:
+                nvram = "<nvram template='%s'>%s</nvram>" % (
+                    nvram_template, self._def['os'].get('nvram'))
 
         disks = ''
         for disk in self._def['devices']['disks']:
@@ -1757,6 +1790,7 @@ class Domain(object):
   <os>
     <type arch='%(arch)s' machine='pc-0.12'>hvm</type>
     %(loader)s
+    %(nvram)s
     <boot dev='hd'/>
   </os>
   <features>
@@ -1808,6 +1842,7 @@ class Domain(object):
                 'iothreads': iothreads,
                 'arch': self._def['os']['arch'],
                 'loader': loader,
+                'nvram': nvram,
                 'disks': disks,
                 'filesystems': filesystems,
                 'nics': nics,
@@ -2697,91 +2732,6 @@ class LibvirtFixture(fixtures.Fixture):
 
         self.useFixture(fixtures.MonkeyPatch('os.path.exists', fake_exists))
 
-        # ...and on all machine types
-        fake_loaders = [
-            {
-                'description': 'UEFI firmware for x86_64',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'flash',
-                    'executable': {
-                        'filename': '/usr/share/OVMF/OVMF_CODE.fd',
-                        'format': 'raw',
-                    },
-                    'nvram-template': {
-                        'filename': '/usr/share/OVMF/OVMF_VARS.fd',
-                        'format': 'raw',
-                    },
-                },
-                'targets': [
-                    {
-                        'architecture': 'x86_64',
-                        'machines': ['pc-i440fx-*', 'pc-q35-*'],
-                    },
-                ],
-                'features': ['acpi-s3', 'amd-sev', 'verbose-dynamic'],
-                'tags': [],
-            },
-            {
-                'description': 'UEFI firmware for x86_64, with SB+SMM',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'flash',
-                    'executable': {
-                        'filename': '/usr/share/OVMF/OVMF_CODE.secboot.fd',
-                        'format': 'raw',
-                    },
-                    'nvram-template': {
-                        'filename': '/usr/share/OVMF/OVMF_VARS.secboot.fd',
-                        'format': 'raw',
-                    },
-                },
-                'targets': [
-                    {
-                        'architecture': 'x86_64',
-                        'machines': ['pc-q35-*'],
-                    },
-                ],
-                'features': [
-                    'acpi-s3',
-                    'amd-sev',
-                    'enrolled-keys',
-                    'requires-smm',
-                    'secure-boot',
-                    'verbose-dynamic',
-                ],
-                'tags': [],
-            },
-            {
-                'description': 'UEFI firmware for aarch64',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'flash',
-                    'executable': {
-                        'filename': '/usr/share/AAVMF/AAVMF_CODE.fd',
-                        'format': 'raw',
-                    },
-                    'nvram-template': {
-                        'filename': '/usr/share/AAVMF/AAVMF_VARS.fd',
-                        'format': 'raw',
-                    }
-                },
-                'targets': [
-                    {
-                        'architecture': 'aarch64',
-                        'machines': ['virt-*'],
-                    }
-                ],
-                'features': ['verbose-static'],
-                "tags": [],
-            },
-        ]
-        self.useFixture(
-            fixtures.MockPatch(
-                'nova.virt.libvirt.host.Host.loaders',
-                new_callable=mock.PropertyMock,
-                return_value=fake_loaders))
-
         disable_event_thread(self)
 
         if self.stub_os_vif:

nova/tests/functional/libvirt/test_uefi.py

@@ -14,8 +14,12 @@
 # under the License.
 
 import datetime
+import os
 import re
+import textwrap
+from unittest import mock
 
+import fixtures
 from lxml import etree
 from oslo_log import log as logging
 from oslo_utils.fixture import uuidsentinel as uuids
@@ -68,14 +72,16 @@ class UEFIServersTest(base.ServersTestBase):
             tree = etree.fromstring(xml)
             self.assertXmlEqual(
                 """
-                <os>
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
                   <type machine='q35'>hvm</type>
-                  <loader type='pflash' readonly='yes' secure='no'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                  <loader secure='no'/>
-                  <nvram template='/usr/share/OVMF/OVMF_VARS.fd'/>
                   <boot dev='hd'/>
                   <smbios mode='sysinfo'/>
                 </os>
-                """,  # noqa: E501
+                """,
                 etree.tostring(tree.find('./os'), encoding='unicode'))
 
             return orig_create(xml, host)
@@ -130,14 +136,16 @@ class UEFIServersTest(base.ServersTestBase):
             tree = etree.fromstring(xml)
             self.assertXmlEqual(
                 """
-                <os>
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='yes'/>
+                  </firmware>
                   <type machine='q35'>hvm</type>
-                  <loader type='pflash' readonly='yes' secure='yes'>/usr/share/OVMF/OVMF_CODE.secboot.fd</loader>
+                  <loader secure='yes'/>
-                  <nvram template='/usr/share/OVMF/OVMF_VARS.secboot.fd'/>
                   <boot dev='hd'/>
                   <smbios mode='sysinfo'/>
                 </os>
-                """,  # noqa: E501
+                """,
                 etree.tostring(tree.find('./os'), encoding='unicode'))
 
             return orig_create(xml, host)
@@ -193,13 +201,16 @@ class UEFIServersTest(base.ServersTestBase):
             tree = etree.fromstring(xml)
             self.assertXmlEqual(
                 """
-                <os>
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
                   <type machine='q35'>hvm</type>
-                  <loader type='pflash' readonly='yes' secure='no' stateless='yes'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                  <loader secure='no' stateless='yes'/>
                   <boot dev='hd'/>
                   <smbios mode='sysinfo'/>
                 </os>
-                """,  # noqa: E501
+                """,
                 etree.tostring(tree.find('./os'), encoding='unicode'))
 
             return orig_create(xml, host)
@@ -255,9 +266,12 @@ class UEFIServersTest(base.ServersTestBase):
             tree = etree.fromstring(xml)
             self.assertXmlEqual(
                 """
-                <os>
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='yes'/>
+                  </firmware>
                   <type machine='q35'>hvm</type>
-                  <loader type='pflash' readonly='yes' secure='yes' stateless='yes'>/usr/share/OVMF/OVMF_CODE.secboot.fd</loader>
+                  <loader secure='yes' stateless='yes'/>
                   <boot dev='hd'/>
                   <smbios mode='sysinfo'/>
                 </os>
@@ -309,3 +323,351 @@ class UEFIServersTest(base.ServersTestBase):
 
         # ensure our instance's system_metadata field is correct
         self.assertInstanceHasUEFI(server, secure_boot=True, stateless=True)
+
+
+class UEFIServersFirmwareTest(base.ServersTestBase):
+
+    def test_hard_reboot(self):
+        orig_path_exists = os.path.exists
+
+        code_exists = True
+        nvram_template_exists = True
+
+        def fake_path_exists(path):
+            if path == '/usr/share/OVMF/OVMF_CODE.fd':
+                return code_exists
+            elif path == '/usr/share/OVMF/OVMF_VARS.fd':
+                return nvram_template_exists
+            else:
+                return orig_path_exists(path)
+
+        self.useFixture(fixtures.MonkeyPatch(
+            'os.path.exists', fake_path_exists))
+
+        orig_create = nova.virt.libvirt.guest.Guest.create
+
+        auto_select = True
+        secure = 'yes'
+
+        def fake_create(cls, xml, host):
+            xml = re.sub('type arch.*machine',
+                'type machine', xml)
+            tree = etree.fromstring(xml)
+            if not auto_select:
+                self.assertXmlEqual(
+                    """
+                    <os>
+                      <type machine='q35'>hvm</type>
+                      <loader type='pflash' readonly='yes' secure='%s'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                      <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/path/to/nvram</nvram>
+                      <boot dev='hd'/>
+                      <smbios mode='sysinfo'/>
+                    </os>
+                    """ % secure,  # noqa: E501
+                    etree.tostring(tree.find('./os'), encoding='unicode'))
+            else:
+                self.assertXmlEqual(
+                    """
+                    <os firmware='efi'>
+                      <firmware>
+                        <feature name='secure-boot' enabled='%s'/>
+                      </firmware>
+                      <type machine='q35'>hvm</type>
+                      <loader secure='%s'/>
+                      <boot dev='hd'/>
+                      <smbios mode='sysinfo'/>
+                    </os>
+                    """ % (secure, secure),
+                    etree.tostring(tree.find('./os'), encoding='unicode'))
+                # NOTE(tkajinam): Simulate edit by libvirt
+                tree.replace(tree.find('./os'), etree.fromstring(
+                    textwrap.dedent("""
+                    <os>
+                      <firmware>
+                        <feature name='secure-boot' enabled='%s'/>
+                      </firmware>
+                      <type machine='q35'>hvm</type>
+                      <loader type='pflash' readonly='yes' secure='%s'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                      <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/path/to/nvram</nvram>
+                      <boot dev='hd'/>
+                      <smbios mode='sysinfo'/>
+                    </os>
+                    """ % (secure, secure),  # noqa: E501
+                )))
+                xml = etree.tostring(tree, encoding='unicode',
+                                     pretty_print=True)
+
+            return orig_create(xml, host)
+
+        self.stub_out('nova.virt.libvirt.guest.Guest.create', fake_create)
+
+        compute = self.start_compute()
+
+        # ensure we are reporting the correct trait
+        traits = self._get_provider_traits(self.compute_rp_uuids[compute])
+        self.assertIn('COMPUTE_SECURITY_UEFI_SECURE_BOOT', traits)
+
+        # create a server with UEFI and secure boot
+        timestamp = datetime.datetime(
+            2021, 1, 2, 3, 4, 5, tzinfo=datetime.timezone.utc
+        )
+        uefi_image = {
+            'id': uuids.uefi_image,
+            'name': 'uefi_image',
+            'created_at': timestamp,
+            'updated_at': timestamp,
+            'deleted_at': None,
+            'deleted': False,
+            'status': 'active',
+            'is_public': False,
+            'container_format': 'ova',
+            'disk_format': 'vhd',
+            'size': 74185822,
+            'min_ram': 0,
+            'min_disk': 0,
+            'protected': False,
+            'visibility': 'public',
+            'tags': [],
+            'properties': {
+                'hw_machine_type': 'q35',
+                'hw_firmware_type': 'uefi',
+                'os_secure_boot': 'optional',
+            }
+        }
+        self.glance.create(None, uefi_image)
+
+        # Initial creation
+        server = self._create_server(image_uuid=uuids.uefi_image)
+        self._stop_server(server)
+
+        # if the domain exists, use the loaders which were already selected
+        auto_select = False
+        self._start_server(server)
+        self._stop_server(server)
+
+        # if code file does not exist, ignore the existing loader
+        code_exists = False
+        auto_select = True
+        self._start_server(server)
+        self._stop_server(server)
+        code_exists = True
+
+        # if nvram template file does not exist, ignore the existing loader
+        nvram_template_exists = False
+        auto_select = True
+        self._start_server(server)
+        self._stop_server(server)
+        nvram_template_exists = True
+
+        # the host lost secure boot support and the secure flag has been
+        # changed from true to false.
+        self.computes[compute].driver._host._supports_secure_boot = False
+        secure = 'no'
+        # if secure boot flag is changed, ignore the existing loader
+        auto_select = True
+        self._start_server(server)
+        self._stop_server(server)
+
+        # if the domain exists, use the loaders which were already selected
+        auto_select = False
+        self._start_server(server)
+        self._stop_server(server)
+
+        # the host regain secure boot support and the secure flag has been
+        # changed from false to true.
+        self.computes[compute].driver._host._supports_secure_boot = True
+        secure = 'yes'
+        # if secure boot flag is changed, ignore the existing loader
+        auto_select = True
+        self._start_server(server)
+
+    def test_rebuild(self):
+        orig_path_exists = os.path.exists
+
+        def fake_path_exists(path):
+            if path == '/usr/share/OVMF/OVMF_CODE.fd':
+                return True
+            elif path == '/usr/share/OVMF/OVMF_VARS.fd':
+                return True
+            else:
+                return orig_path_exists(path)
+
+        self.useFixture(fixtures.MonkeyPatch(
+            'os.path.exists', fake_path_exists))
+
+        orig_create = nova.virt.libvirt.guest.Guest.create
+
+        def fake_create(cls, xml, host):
+            xml = re.sub('type arch.*machine',
+                'type machine', xml)
+            tree = etree.fromstring(xml)
+            self.assertXmlEqual(
+                """
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
+                  <type machine='q35'>hvm</type>
+                  <loader secure='no'/>
+                  <boot dev='hd'/>
+                  <smbios mode='sysinfo'/>
+                </os>
+                """,
+                etree.tostring(tree.find('./os'), encoding='unicode'))
+            # NOTE(tkajinam): Simulate edit by libvirt
+            tree.replace(tree.find('./os'), etree.fromstring(
+                textwrap.dedent("""
+                <os>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
+                  <type machine='q35'>hvm</type>
+                  <loader type='pflash' readonly='yes' secure='no'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                  <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/path/to/nvram</nvram>
+                  <boot dev='hd'/>
+                  <smbios mode='sysinfo'/>
+                </os>
+                """,  # noqa: E501
+            )))
+            xml = etree.tostring(tree, encoding='unicode', pretty_print=True)
+
+            return orig_create(xml, host)
+
+        self.stub_out('nova.virt.libvirt.guest.Guest.create', fake_create)
+
+        compute = self.start_compute()
+
+        # ensure we are reporting the correct trait
+        traits = self._get_provider_traits(self.compute_rp_uuids[compute])
+        self.assertIn('COMPUTE_SECURITY_UEFI_SECURE_BOOT', traits)
+
+        # create a server with UEFI
+        timestamp = datetime.datetime(
+            2021, 1, 2, 3, 4, 5, tzinfo=datetime.timezone.utc
+        )
+        uefi_image = {
+            'id': uuids.uefi_image,
+            'name': 'uefi_image',
+            'created_at': timestamp,
+            'updated_at': timestamp,
+            'deleted_at': None,
+            'deleted': False,
+            'status': 'active',
+            'is_public': False,
+            'container_format': 'ova',
+            'disk_format': 'vhd',
+            'size': 74185822,
+            'min_ram': 0,
+            'min_disk': 0,
+            'protected': False,
+            'visibility': 'public',
+            'tags': [],
+            'properties': {
+                'hw_machine_type': 'q35',
+                'hw_firmware_type': 'uefi',
+            }
+        }
+        self.glance.create(None, uefi_image)
+
+        # Initial creation
+        server = self._create_server(image_uuid=uuids.uefi_image)
+
+        # In rebuild, the previous xml is destroyed thus firmware is again
+        # auto-selected.
+        self._rebuild_server(server, uuids.uefi_image)
+
+    def test_resize(self):
+        orig_path_exists = os.path.exists
+
+        def fake_path_exists(path):
+            if path == '/usr/share/OVMF/OVMF_CODE.fd':
+                return True
+            elif path == '/usr/share/OVMF/OVMF_VARS.fd':
+                return True
+            else:
+                return orig_path_exists(path)
+
+        self.useFixture(fixtures.MonkeyPatch(
+            'os.path.exists', fake_path_exists))
+
+        orig_create = nova.virt.libvirt.guest.Guest.create
+
+        def fake_create(cls, xml, host):
+            xml = re.sub('type arch.*machine',
+                'type machine', xml)
+            tree = etree.fromstring(xml)
+            self.assertXmlEqual(
+                """
+                <os firmware='efi'>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
+                  <type machine='q35'>hvm</type>
+                  <loader secure='no'/>
+                  <boot dev='hd'/>
+                  <smbios mode='sysinfo'/>
+                </os>
+                """,
+                etree.tostring(tree.find('./os'), encoding='unicode'))
+            # NOTE(tkajinam): Simulate edit by libvirt
+            tree.replace(tree.find('./os'), etree.fromstring(
+                textwrap.dedent("""
+                <os>
+                  <firmware>
+                    <feature name='secure-boot' enabled='no'/>
+                  </firmware>
+                  <type machine='q35'>hvm</type>
+                  <loader type='pflash' readonly='yes' secure='no'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+                  <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/path/to/nvram</nvram>
+                  <boot dev='hd'/>
+                  <smbios mode='sysinfo'/>
+                </os>
+                """,  # noqa: E501
+            )))
+            xml = etree.tostring(tree, encoding='unicode', pretty_print=True)
+
+            return orig_create(xml, host)
+
+        self.stub_out('nova.virt.libvirt.guest.Guest.create', fake_create)
+
+        self.start_compute('host1')
+        self.start_compute('host2')
+
+        # create a server with UEFI
+        timestamp = datetime.datetime(
+            2021, 1, 2, 3, 4, 5, tzinfo=datetime.timezone.utc
+        )
+        uefi_image = {
+            'id': uuids.uefi_image,
+            'name': 'uefi_image',
+            'created_at': timestamp,
+            'updated_at': timestamp,
+            'deleted_at': None,
+            'deleted': False,
+            'status': 'active',
+            'is_public': False,
+            'container_format': 'ova',
+            'disk_format': 'vhd',
+            'size': 74185822,
+            'min_ram': 0,
+            'min_disk': 0,
+            'protected': False,
+            'visibility': 'public',
+            'tags': [],
+            'properties': {
+                'hw_machine_type': 'q35',
+                'hw_firmware_type': 'uefi',
+            }
+        }
+        self.glance.create(None, uefi_image)
+
+        # Initial creation
+        server = self._create_server(image_uuid=uuids.uefi_image)
+
+        # In cold-migration, the previous xml is destroyed so firmware should
+        # be auto-selected.
+        with mock.patch(
+            'nova.virt.libvirt.driver.LibvirtDriver'
+            '.migrate_disk_and_power_off', return_value='{}',
+        ):
+            self._resize_server(server, self.api.get_flavors()[1]['id'])

nova/tests/unit/virt/libvirt/test_driver.py

@@ -6084,6 +6084,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
 
     def test_get_guest_config_with_uefi(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
 
         image_meta = objects.ImageMeta.from_dict({
             "disk_format": "raw",
@@ -6094,12 +6096,145 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             CONF.libvirt.virt_type, instance_ref, image_meta)
         cfg = drvr._get_guest_config(
             instance_ref, [], image_meta, disk_info)
-        # these paths are derived from the FakeLibvirtFixture
+
-        self.assertEqual('/usr/share/OVMF/OVMF_CODE.fd', cfg.os_loader)
+        self.assertEqual('efi', cfg.os_firmware)
-        self.assertEqual('/usr/share/OVMF/OVMF_VARS.fd', cfg.os_nvram_template)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
+        self.assertIsNone(cfg.os_nvram_template)
+
+    @mock.patch('nova.virt.libvirt.driver.os.path.exists')
+    def test_get_guest_config_with_uefi_old_guest(self, mock_exists):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
+
+        image_meta = objects.ImageMeta.from_dict({
+            "disk_format": "raw",
+            "properties": {"hw_firmware_type": "uefi"}})
+        instance_ref = objects.Instance(**self.test_instance)
+
+        loader = 'LOADER'
+        nvram = 'NVRAM'
+        nvram_template = 'NVRAM_TEMPLATE'
+
+        old_guest = vconfig.LibvirtConfigGuest()
+        old_guest.os_loader_type = 'pflash'
+        old_guest.os_loader_readonly = True
+        old_guest.os_loader_secure = False
+        old_guest.os_loader = loader
+        old_guest.os_nvram = nvram
+        old_guest.os_nvram_template = nvram_template
+
+        mock_exists.return_value = True
+
+        disk_info = blockinfo.get_disk_info(
+            CONF.libvirt.virt_type, instance_ref, image_meta)
+        cfg = drvr._get_guest_config(
+            instance_ref, [], image_meta, disk_info, old_guest=old_guest)
+
+        self.assertIsNone(cfg.os_firmware)
+        self.assertEqual('pflash', cfg.os_loader_type)
+        self.assertTrue(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertEqual(loader, cfg.os_loader)
+        self.assertEqual(nvram, cfg.os_nvram)
+        self.assertEqual(nvram_template, cfg.os_nvram_template)
+
+    @mock.patch('nova.virt.libvirt.driver.os.path.exists')
+    def test_get_guest_config_with_uefi_old_guest_loader_not_found(
+            self, mock_exists):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
+
+        image_meta = objects.ImageMeta.from_dict({
+            "disk_format": "raw",
+            "properties": {"hw_firmware_type": "uefi"}})
+        instance_ref = objects.Instance(**self.test_instance)
+
+        loader = 'LOADER'
+        nvram = 'NVRAM'
+        nvram_template = 'NVRAM_TEMPLATE'
+
+        old_guest = vconfig.LibvirtConfigGuest()
+        old_guest.os_loader_type = 'pflash'
+        old_guest.os_loader_readonly = True
+        old_guest.os_loader_secure = False
+        old_guest.os_loader = loader
+        old_guest.os_nvram = nvram
+        old_guest.os_nvram_template = nvram_template
+
+        def mock_func(path):
+            return path != 'LOADER'
+
+        mock_exists.side_effect = mock_func
+
+        disk_info = blockinfo.get_disk_info(
+            CONF.libvirt.virt_type, instance_ref, image_meta)
+        cfg = drvr._get_guest_config(
+            instance_ref, [], image_meta, disk_info, old_guest=old_guest)
+
+        self.assertEqual('efi', cfg.os_firmware)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
+        self.assertIsNone(cfg.os_nvram_template)
+
+    @mock.patch('nova.virt.libvirt.driver.os.path.exists')
+    def test_get_guest_config_with_uefi_old_guest_nvram_not_found(
+            self, mock_exists):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
+
+        image_meta = objects.ImageMeta.from_dict({
+            "disk_format": "raw",
+            "properties": {"hw_firmware_type": "uefi"}})
+        instance_ref = objects.Instance(**self.test_instance)
+
+        loader = 'LOADER'
+        nvram = 'NVRAM'
+        nvram_template = 'NVRAM_TEMPLATE'
+
+        old_guest = vconfig.LibvirtConfigGuest()
+        old_guest.os_loader_type = 'pflash'
+        old_guest.os_loader_readonly = True
+        old_guest.os_loader_secure = False
+        old_guest.os_loader = loader
+        old_guest.os_nvram = nvram
+        old_guest.os_nvram_template = nvram_template
+
+        def mock_func(path):
+            return path != 'NVRAM_TEMPLATE'
+
+        mock_exists.side_effect = mock_func
+
+        disk_info = blockinfo.get_disk_info(
+            CONF.libvirt.virt_type, instance_ref, image_meta)
+        cfg = drvr._get_guest_config(
+            instance_ref, [], image_meta, disk_info, old_guest=old_guest)
+
+        self.assertEqual('efi', cfg.os_firmware)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
+        self.assertIsNone(cfg.os_nvram_template)
 
     def test_get_guest_config_with_uefi_and_stateless_firmware(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
 
         image_meta = objects.ImageMeta.from_dict({
             "disk_format": "raw",
@@ -6114,54 +6249,22 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             CONF.libvirt.virt_type, instance_ref, image_meta)
         cfg = drvr._get_guest_config(
             instance_ref, [], image_meta, disk_info)
-        # these paths are derived from the FakeLibvirtFixture
+
-        self.assertEqual('/usr/share/OVMF/OVMF_CODE.fd', cfg.os_loader)
+        self.assertEqual('efi', cfg.os_firmware)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
         self.assertTrue(cfg.os_loader_stateless)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
         self.assertIsNone(cfg.os_nvram_template)
 
-    def test_get_guest_config_with_secure_boot_and_smm_required(self):
-        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
-        # uefi only used with secure boot
-        drvr._host._supports_uefi = True
-        # smm only used with secure boot
-        drvr._host._supports_secure_boot = True
-
-        # NOTE(imranh2): Current way of gathering firmwares is inflexible
-        # nova/tests/fixtures/libvirt.py FakeLoaders has requires-smm
-        # defined. do the following to make sure we get this programtically
-        # in the future we should test firmwares that both do and don't
-        # require smm but the current way firmware is selected doesn't
-        # make it possible to do so.
-        loader, nvram_template, requires_smm = drvr._host.get_loader(
-            'x86_64', 'q35', True)
-
-        image_meta = objects.ImageMeta.from_dict({
-            'disk_format': 'raw',
-            # secure boot requires UEFI
-            'properties': {
-                'hw_firmware_type': 'uefi',
-                'hw_machine_type': 'q35',
-                'os_secure_boot': 'required',
-            },
-        })
-        instance_ref = objects.Instance(**self.test_instance)
-
-        disk_info = blockinfo.get_disk_info(
-            CONF.libvirt.virt_type, instance_ref, image_meta)
-
-        cfg = drvr._get_guest_config(
-            instance_ref, [], image_meta, disk_info)
-        # if we require it make sure it's there
-        if requires_smm:
-            self.assertTrue(any(isinstance(feature,
-              vconfig.LibvirtConfigGuestFeatureSMM)
-                for feature in cfg.features))
-
     @ddt.data(True, False)
     def test_get_guest_config_with_secure_boot_required(
         self, host_has_support,
     ):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
         drvr._host._supports_uefi = True
         drvr._host._supports_secure_boot = host_has_support
 
@@ -6182,12 +6285,13 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             # if the host supports it, we should get the feature
             cfg = drvr._get_guest_config(
                 instance_ref, [], image_meta, disk_info)
-            # these paths are derived from the FakeLibvirtFixture
+            self.assertEqual('efi', cfg.os_firmware)
-            self.assertEqual(
+            self.assertIsNone(cfg.os_loader_type)
-                '/usr/share/OVMF/OVMF_CODE.secboot.fd', cfg.os_loader)
+            self.assertIsNone(cfg.os_loader_readonly)
-            self.assertEqual(
-                '/usr/share/OVMF/OVMF_VARS.secboot.fd', cfg.os_nvram_template)
             self.assertTrue(cfg.os_loader_secure)
+            self.assertIsNone(cfg.os_loader)
+            self.assertIsNone(cfg.os_nvram)
+            self.assertIsNone(cfg.os_nvram_template)
         else:
             # if not, we should see an exception
             self.assertRaises(
@@ -6200,6 +6304,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         self, host_has_support,
     ):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
         drvr._host._supports_uefi = True
         drvr._host._supports_secure_boot = host_has_support
 
@@ -6219,20 +6324,119 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         cfg = drvr._get_guest_config(
             instance_ref, [], image_meta, disk_info)
         if host_has_support:
-            # if the host supports it we should get the feature
-            self.assertEqual(
-                '/usr/share/OVMF/OVMF_CODE.secboot.fd', cfg.os_loader)
-            self.assertEqual(
-                '/usr/share/OVMF/OVMF_VARS.secboot.fd', cfg.os_nvram_template)
             self.assertTrue(cfg.os_loader_secure)
         else:
-            # if not, silently ignore
-            self.assertEqual(
-                '/usr/share/OVMF/OVMF_CODE.fd', cfg.os_loader)
-            self.assertEqual(
-                '/usr/share/OVMF/OVMF_VARS.fd', cfg.os_nvram_template)
             self.assertFalse(cfg.os_loader_secure)
 
+        self.assertEqual('efi', cfg.os_firmware)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
+        self.assertIsNone(cfg.os_nvram_template)
+        self.assertNotIn(vconfig.LibvirtConfigGuestFeatureSMM(), cfg.features)
+
+    @ddt.data(True, False)
+    @mock.patch('nova.virt.libvirt.driver.os.path.exists')
+    def test_get_guest_config_with_secure_boot_old_guest(
+            self, mock_exists, smm):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
+        drvr._host._supports_secure_boot = True
+
+        image_meta = objects.ImageMeta.from_dict({
+            'disk_format': 'raw',
+            # secure boot requires UEFI
+            'properties': {
+                'hw_firmware_type': 'uefi',
+                'hw_machine_type': 'q35',
+                'os_secure_boot': 'required',
+            },
+        })
+        instance_ref = objects.Instance(**self.test_instance)
+
+        loader = 'LOADER'
+        nvram = 'NVRAM'
+        nvram_template = 'NVRAM_TEMPLATE'
+
+        old_guest = vconfig.LibvirtConfigGuest()
+        old_guest.os_loader_type = 'pflash'
+        old_guest.os_loader_readonly = True
+        old_guest.os_loader_secure = True
+        old_guest.os_loader = loader
+        old_guest.os_nvram = nvram
+        old_guest.os_nvram_template = nvram_template
+        if smm:
+            old_guest.features.append(vconfig.LibvirtConfigGuestFeatureSMM())
+
+        disk_info = blockinfo.get_disk_info(
+            CONF.libvirt.virt_type, instance_ref, image_meta)
+        cfg = drvr._get_guest_config(
+            instance_ref, [], image_meta, disk_info, old_guest=old_guest)
+        self.assertEqual('pflash', cfg.os_loader_type)
+        self.assertTrue(cfg.os_loader_readonly)
+        self.assertTrue(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertEqual(loader, cfg.os_loader)
+        self.assertEqual(nvram, cfg.os_nvram)
+        self.assertEqual(nvram_template, cfg.os_nvram_template)
+        if smm:
+            self.assertIn(vconfig.LibvirtConfigGuestFeatureSMM(),
+                          cfg.features)
+        else:
+            self.assertNotIn(vconfig.LibvirtConfigGuestFeatureSMM(),
+                             cfg.features)
+
+    @mock.patch('nova.virt.libvirt.driver.os.path.exists')
+    def test_get_guest_config_with_uefi_old_guest_sb_changed(
+            self, mock_exists):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
+        drvr._host._supports_amd_sev = False
+        drvr._host._supports_uefi = True
+        drvr._host._supports_secure_boot = False
+
+        image_meta = objects.ImageMeta.from_dict({
+            "disk_format": "raw",
+            "properties": {
+                'hw_firmware_type': 'uefi',
+                'hw_machine_type': 'q35',
+                'os_secure_boot': 'optional',
+            }
+        })
+        instance_ref = objects.Instance(**self.test_instance)
+
+        loader = 'LOADER'
+        nvram = 'NVRAM'
+        nvram_template = 'NVRAM_TEMPLATE'
+
+        # Secure boot was enabled previously
+        old_guest = vconfig.LibvirtConfigGuest()
+        old_guest.os_loader_type = 'pflash'
+        old_guest.os_loader_readonly = True
+        old_guest.os_loader_secure = True
+        old_guest.os_loader = loader
+        old_guest.os_nvram = nvram
+        old_guest.os_nvram_template = nvram_template
+        old_guest.features.append(vconfig.LibvirtConfigGuestFeatureSMM())
+
+        mock_exists.return_value = True
+
+        disk_info = blockinfo.get_disk_info(
+            CONF.libvirt.virt_type, instance_ref, image_meta)
+        cfg = drvr._get_guest_config(
+            instance_ref, [], image_meta, disk_info, old_guest=old_guest)
+
+        self.assertEqual('efi', cfg.os_firmware)
+        self.assertIsNone(cfg.os_loader_type)
+        self.assertIsNone(cfg.os_loader_readonly)
+        self.assertFalse(cfg.os_loader_secure)
+        self.assertIsNone(cfg.os_loader_stateless)
+        self.assertIsNone(cfg.os_loader)
+        self.assertIsNone(cfg.os_nvram)
+        self.assertIsNone(cfg.os_nvram_template)
+        self.assertNotIn(vconfig.LibvirtConfigGuestFeatureSMM(), cfg.features)
+
     def test_get_guest_config_with_block_device(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
 
@@ -18527,9 +18731,13 @@ class LibvirtConnTestCase(test.NoDBTestCase,
     @mock.patch('nova.virt.libvirt.LibvirtDriver.destroy')
     @mock.patch('nova.virt.libvirt.LibvirtDriver.'
                 '_get_all_assigned_mediated_devices')
-    def test_hard_reboot(self, mock_get_mdev, mock_destroy, mock_get_disk_info,
+    @mock.patch('nova.virt.libvirt.LibvirtDriver.'
-                         mock_get_guest_xml, mock_create_guest_with_network,
+                '_get_existing_guest_config')
-                         mock_get_info, mock_metadata, mock_save):
+    def test_hard_reboot(
+        self, mock_get_guest, mock_get_mdev, mock_destroy, mock_get_disk_info,
+        mock_get_guest_xml, mock_create_guest_with_network,
+        mock_get_info, mock_metadata, mock_save
+    ):
         self.context.auth_token = True  # any non-None value will suffice
         instance = objects.Instance(**self.test_instance)
         network_info = _fake_network_info(self)
@@ -18545,6 +18753,10 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                     "<target dev='vdb' bus='virtio'/></disk>"
                     "</devices></domain>")
 
+        guest = vconfig.LibvirtConfigGuest()
+        guest.parse_dom(etree.fromstring(dummyxml))
+        mock_get_guest.return_value = guest
+
         mock_get_mdev.return_value = {uuids.mdev1: uuids.inst1}
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
@@ -18595,7 +18807,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         mock_get_guest_xml.assert_called_once_with(self.context, instance,
             network_info, mock.ANY, mock.ANY,
             block_device_info=block_device_info, mdevs=[uuids.mdev1],
-            accel_info=accel_info, share_info=share_info)
+            accel_info=accel_info, share_info=share_info,
+            old_guest=guest)
         mock_create_guest_with_network.assert_called_once_with(
                 self.context, dummyxml, instance, network_info,
                 block_device_info, vifs_already_plugged=True,
@@ -18615,8 +18828,10 @@ class LibvirtConnTestCase(test.NoDBTestCase,
     @mock.patch('nova.virt.libvirt.LibvirtDriver.destroy')
     @mock.patch('nova.virt.libvirt.LibvirtDriver.'
                 '_get_all_assigned_mediated_devices')
+    @mock.patch('nova.virt.libvirt.LibvirtDriver.'
+                '_get_existing_guest_config')
     def test_hard_reboot_with_share_info(
-        self, mock_get_mdev, mock_destroy, mock_get_disk_info,
+        self, mock_get_guest, mock_get_mdev, mock_destroy, mock_get_disk_info,
         mock_get_guest_xml, mock_create_guest_with_network,
         mock_get_info, mock_attach, mock_metadata, mock_save
     ):
@@ -18635,6 +18850,10 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                     "<target dev='vdb' bus='virtio'/></disk>"
                     "</devices></domain>")
 
+        guest = vconfig.LibvirtConfigGuest()
+        guest.parse_dom(etree.fromstring(dummyxml))
+        mock_get_guest.return_value = guest
+
         mock_get_mdev.return_value = {uuids.mdev1: uuids.inst1}
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
@@ -18700,7 +18919,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         mock_get_guest_xml.assert_called_once_with(self.context, instance,
             network_info, mock.ANY, mock.ANY,
             block_device_info=block_device_info, mdevs=[uuids.mdev1],
-            accel_info=accel_info, share_info=share_info)
+            accel_info=accel_info, share_info=share_info,
+            old_guest=guest)
         mock_create_guest_with_network.assert_called_once_with(
             self.context, dummyxml, instance, network_info, block_device_info,
             vifs_already_plugged=True,
@@ -25515,7 +25735,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
 
         def fake_to_xml(self, context, instance, network_info, disk_info,
                         image_meta=None, rescue=None,
-                        block_device_info=None, mdevs=None):
+                        block_device_info=None, mdevs=None, old_guest=None):
             return ""
 
         self.stub_out('nova.virt.libvirt.driver.LibvirtDriver._get_guest_xml',
@@ -25543,16 +25763,21 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             f = open(libvirt_xml_path, 'w')
             f.close()
 
-            with mock.patch.object(
+            with test.nested(
-                self.drvr, '_get_all_assigned_mediated_devices',
+                mock.patch.object(
-                return_value={}
+                    self.drvr, '_get_all_assigned_mediated_devices',
-            ) as mock_get_a_mdevs:
+                    return_value={}),
+                mock.patch.object(
+                    self.drvr, '_get_existing_guest_config',
+                    return_value=None),
+            ) as (mock_get_a_mdevs, mock_get_guest):
                 self.drvr.finish_revert_migration(
                     self.context, instance, network_model.NetworkInfo(),
                         objects.Migration(), power_on=power_on)
 
             self.assertTrue(self.fake_create_guest_called)
             mock_get_a_mdevs.assert_called_once_with(mock.ANY)
+            mock_get_guest.assert_called_once_with(mock.ANY)
 
     def test_finish_revert_migration_power_on(self):
         self._test_finish_revert_migration(True)
@@ -25619,7 +25844,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
     def test_finish_revert_migration_preserves_disk_bus(self):
 
         def fake_get_guest_xml(context, instance, network_info, disk_info,
-                               image_meta, block_device_info=None, mdevs=None):
+                               image_meta, block_device_info=None, mdevs=None,
+                               old_guest=None):
             self.assertEqual('ide', disk_info['disk_bus'])
 
         image_meta = {"disk_format": "raw",
@@ -27527,6 +27753,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
     @mock.patch('nova.objects.block_device.BlockDeviceMapping.save',
                 new=mock.Mock())
     @mock.patch('nova.objects.image_meta.ImageMeta.from_image_ref')
+    @mock.patch('nova.virt.libvirt.LibvirtDriver.'
+                '_get_existing_guest_config')
     @mock.patch('nova.virt.libvirt.LibvirtDriver.'
                 '_get_all_assigned_mediated_devices')
     # NOTE(mdbooth): The following 4 mocks are required to execute
@@ -27539,8 +27767,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
     def _test_rescue(
         self, instance, mock_instance_metadata, mock_supports_direct_io,
         mock_build_device_metadata, mock_set_host_enabled, mock_get_mdev,
-        mock_get_image_meta_by_ref, image_meta_dict=None, exists=None,
+        mock_get_guest, mock_get_image_meta_by_ref, image_meta_dict=None,
-        instance_image_meta_dict=None, block_device_info=None, share_info=None
+        exists=None, instance_image_meta_dict=None, block_device_info=None,
+        share_info=None
     ):
 
         self.flags(instances_path=self.useFixture(fixtures.TempDir()).path)
@@ -27548,6 +27777,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         mock_supports_direct_io.return_value = True
 
         mock_get_mdev.return_value = {uuids.mdev1: uuids.inst1}
+        mock_get_guest.return_value = None
 
         backend = self.useFixture(
             nova_fixtures.LibvirtImageBackendFixture(exists=exists))
@@ -27856,6 +28086,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             mock.patch.object(drvr, '_destroy'),
             mock.patch.object(drvr, '_get_guest_xml'),
             mock.patch.object(drvr, '_create_image'),
+            mock.patch.object(drvr, '_get_existing_guest_config'),
             mock.patch.object(drvr, '_get_existing_domain_xml'),
             mock.patch.object(libvirt_utils, 'get_instance_path'),
             mock.patch('nova.virt.libvirt.blockinfo.get_disk_info'),
@@ -27864,10 +28095,12 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             mock.patch('builtins.open', new_callable=mock.mock_open),
         ) as (
             mock_create, mock_destroy, mock_get_guest_xml, mock_create_image,
-            mock_get_existing_xml, mock_inst_path, mock_get_disk_info,
+            mock_get_existing_guest, mock_get_existing_xml,
-            mock_image_get, mock_from_dict, mock_open
+            mock_inst_path, mock_get_disk_info, mock_image_get, mock_from_dict,
+            mock_open
         ):
             self.flags(virt_type='kvm', group='libvirt')
+            mock_get_existing_guest.return_value = None
             mock_image_get.return_value = mock.sentinel.bdm_image_meta_dict
             mock_from_dict.return_value = mock.sentinel.bdm_image_meta
             mock_get_disk_info.return_value = disk_info
@@ -27896,7 +28129,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             mock_get_guest_xml.assert_called_once_with(
                 self.context, instance, network_info, disk_info,
                 mock.sentinel.bdm_image_meta, rescue=mock.ANY, mdevs=mock.ANY,
-                block_device_info=block_device_info, share_info=share_info)
+                block_device_info=block_device_info, share_info=share_info,
+                old_guest=None)
 
     def test_rescue_stable_device_bfv(self):
         """Assert the disk layout when rescuing BFV instances"""
@@ -29417,6 +29651,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
 
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_build_device_metadata')
     @mock.patch.object(objects.Instance, 'save')
+    @mock.patch.object(libvirt_driver.LibvirtDriver,
+                       '_get_existing_guest_config')
     @mock.patch.object(
         libvirt_driver.LibvirtDriver, '_get_all_assigned_mediated_devices',
         new=mock.Mock(return_value={}))
@@ -29436,7 +29672,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         'oslo_service.loopingcall.FixedIntervalLoopingCall', new=mock.Mock())
     def _test_hard_reboot_allocate_missing_mdevs(
             self, mock_get_xml, mock_image_meta, mock_allocate_mdevs,
-            mock_db, mock_build_metadata):
+            mock_get_guest, mock_db, mock_build_metadata):
         mock_compute = mock.Mock()
         mock_compute.reportclient.get_allocations_for_consumer.return_value = (
             mock.sentinel.allocations)
@@ -29449,6 +29685,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             image_ref=uuids.image,
             flavor=objects.Flavor(extra_specs={'resources:VGPU': 1}))
 
+        old_guest = mock.Mock()
+        mock_get_guest.return_value = old_guest
+
         share_info = objects.ShareMappingList()
         mock_build_metadata.return_value = objects.InstanceDeviceMetadata()
         drvr._hard_reboot(
@@ -29458,6 +29697,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         (mock_compute.reportclient.get_allocations_for_consumer.
             assert_called_once_with(ctxt, instance.uuid))
         mock_allocate_mdevs.assert_called_once_with(mock.sentinel.allocations)
+        mock_get_guest.assert_called_once_with(instance)
         mock_get_xml.assert_called_once_with(
             ctxt,
             instance,
@@ -29468,6 +29708,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
             mdevs=mock_allocate_mdevs.return_value,
             accel_info=None,
             share_info=share_info,
+            old_guest=old_guest,
         )
 
         return ctxt, mock_get_xml, instance

nova/tests/unit/virt/libvirt/test_host.py

@@ -21,7 +21,6 @@ import ddt
 import eventlet
 from eventlet import tpool
 from lxml import etree
-from oslo_serialization import jsonutils
 from oslo_utils.fixture import uuidsentinel as uuids
 from oslo_utils import uuidutils
 from oslo_utils import versionutils
@@ -2092,133 +2091,6 @@ Active:          8381604 kB
         mock_libversion.return_value = 7008000
         self.assertFalse(self.host.supports_remote_managed_ports)
 
-    @mock.patch.object(host.Host, 'loaders', new_callable=mock.PropertyMock)
-    @mock.patch.object(host.Host, 'get_canonical_machine_type')
-    def test_get_loader(self, mock_get_mtype, mock_loaders):
-        loaders = [
-            {
-                'description': 'Sample descriptor',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'flash',
-                    'mode': 'split',
-                    'executable': {
-                        'filename': '/usr/share/edk2/ovmf/OVMF_CODE.fd',
-                        'format': 'raw',
-                    },
-                    'nvram-template': {
-                        'filename': '/usr/share/edk2/ovmf/OVMF_VARS.fd',
-                        'format': 'raw',
-                    },
-                },
-                'targets': [
-                    {
-                        'architecture': 'x86_64',
-                        'machines': ['pc-q35-*'],  # exclude pc-i440fx-*
-                    },
-                ],
-                'features': ['acpi-s3', 'amd-sev', 'verbose-dynamic'],
-                'tags': [],
-            },
-            # NOTE(tkajinam): The following loaders are not supported and
-            # should be ignored. https://bugs.launchpad.net/nova/+bug/2122288
-            {
-                'description': 'Sample descriptor for stateless mode',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'flash',
-                    'mode': 'stateless',
-                    'executable': {
-                        'filename': '/usr/share/edk2/ovmf/OVMF_CODE_SL.fd',
-                        'format': 'raw'
-                    }
-                },
-                'targets': [
-                    {
-                        'architecture': 'x86_64',
-                        'machines': ['pc-q35-*'],
-                    },
-                ],
-                'features': ['amd-sev', 'verbose-dynamic'],
-                'tags': [],
-            },
-            {
-                'description': 'Sample descriptor for memory device',
-                'interface-types': ['uefi'],
-                'mapping': {
-                    'device': 'memory',
-                    'filename': '/usr/share/edk2/ovmf/OVMF_MEM.fd'
-                },
-                'targets': [
-                    {
-                        'architecture': 'x86_64',
-                        'machines': ['pc-q35-*'],
-                    }
-                ],
-                'features': ['amd-sev', 'verbose-dynamic'],
-                'tags': [],
-            },
-        ]
-
-        def fake_get_mtype(arch, machine):
-            return {
-                'x86_64': {
-                    'pc': 'pc-i440fx-5.1',
-                    'q35': 'pc-q35-5.1',
-                },
-                'aarch64': {
-                    'virt': 'virt-5.1',
-                },
-            }[arch][machine]
-
-        mock_get_mtype.side_effect = fake_get_mtype
-        mock_loaders.return_value = loaders
-
-        # this should pass because we're not reporting the secure-boot feature
-        # which is what we don't want
-        loader = self.host.get_loader('x86_64', 'q35', has_secure_boot=False)
-        self.assertIsNotNone(loader)
-
-        # while it should fail here since we want it now
-        self.assertRaises(
-            exception.UEFINotSupported,
-            self.host.get_loader,
-            'x86_64', 'q35', has_secure_boot=True)
-
-        # it should also fail for an unsupported architecture
-        self.assertRaises(
-            exception.UEFINotSupported,
-            self.host.get_loader,
-            'aarch64', 'virt', has_secure_boot=False)
-
-        # or an unsupported machine type
-        self.assertRaises(
-            exception.UEFINotSupported,
-            self.host.get_loader,
-            'x86_64', 'pc', has_secure_boot=False)
-
-        # add the secure-boot feature flag
-        loaders[0]['features'].append('secure-boot')
-
-        # this should pass because we're reporting the secure-boot feature
-        # which is what we want
-        loader = self.host.get_loader('x86_64', 'q35', has_secure_boot=True)
-        self.assertIsNotNone(loader)
-
-        # check that SMM bool is false as we don't need it
-        self.assertFalse(loader[2])
-
-        # check that we get SMM bool correctly (True) when required
-        loaders[0]['features'].append('requires-smm')
-        loader = self.host.get_loader('x86_64', 'q35', has_secure_boot=True)
-        self.assertTrue(loader[2])
-
-        # while it should fail here since we don't want it now
-        self.assertRaises(
-            exception.UEFINotSupported,
-            self.host.get_loader,
-            'x86_64', 'q35', has_secure_boot=False)
-
 
 vc = fakelibvirt.virConnect
 
@@ -2493,51 +2365,3 @@ class LibvirtTpoolProxyTestCase(test.NoDBTestCase):
         self.assertEqual(1, len(dev_names))
         for name in dev_names:
             self.assertIsInstance(name, str)
-
-
-class LoadersTestCase(test.NoDBTestCase):
-
-    def test_loaders(self):
-        loader = {
-            'description': 'Sample descriptor',
-            'interface-types': ['uefi'],
-            'mapping': {
-                'device': 'flash',
-                'executable': {
-                    'filename': '/usr/share/edk2/ovmf/OVMF_CODE.fd',
-                    'format': 'raw',
-                },
-                'nvram-template': {
-                    'filename': '/usr/share/edk2/ovmf/OVMF_VARS.fd',
-                    'format': 'raw',
-                },
-            },
-            'targets': [
-                {
-                    'architecture': 'x86_64',
-                    'machines': ['pc-i440fx-*', 'pc-q35-*'],
-                },
-            ],
-            'features': ['acpi-s3', 'amd-sev', 'verbose-dynamic'],
-            'tags': [],
-        }
-
-        m = mock.mock_open(read_data=jsonutils.dumps(loader).encode('utf-8'))
-        with test.nested(
-            mock.patch.object(
-                os.path, 'exists',
-                side_effect=lambda path: path == '/usr/share/qemu/firmware'),
-            mock.patch('glob.glob', return_value=['10_fake.json']),
-            mock.patch('builtins.open', m, create=True),
-        ) as (mock_exists, mock_glob, mock_open):
-            loaders = host._get_loaders()
-
-            self.assertEqual(loaders, [loader])
-
-            mock_exists.assert_has_calls([
-                mock.call('/usr/share/qemu/firmware'),
-                mock.call('/etc/qemu/firmware'),
-            ])
-            mock_glob.assert_called_once_with(
-                '/usr/share/qemu/firmware/*.json')
-            mock_open.assert_called_once_with('10_fake.json', 'rb')

nova/virt/libvirt/driver.py

@@ -4146,6 +4146,9 @@ class LibvirtDriver(driver.ComputeDriver):
         # need to remember the existing mdevs for reusing them.
         mdevs = self._get_all_assigned_mediated_devices(instance)
         mdevs = list(mdevs.keys())
+
+        old_guest = self._get_existing_guest_config(instance)
+
         # NOTE(mdbooth): In addition to performing a hard reboot of the domain,
         # the hard reboot operation is relied upon by operators to be an
         # automated attempt to fix as many things as possible about a
@@ -4191,7 +4194,8 @@ class LibvirtDriver(driver.ComputeDriver):
                                   instance.image_meta,
                                   block_device_info=block_device_info,
                                   mdevs=mdevs, accel_info=accel_info,
-                                  share_info=share_info)
+                                  share_info=share_info,
+                                  old_guest=old_guest)
 
         # NOTE(mdbooth): context.auth_token will not be set when we call
         #                _hard_reboot from resume_state_on_host_boot()
@@ -4655,6 +4659,9 @@ class LibvirtDriver(driver.ComputeDriver):
         # remember the existing mdevs for reusing them.
         mdevs = self._get_all_assigned_mediated_devices(instance)
         mdevs = list(mdevs.keys())
+
+        old_guest = self._get_existing_guest_config(instance)
+
         self._create_image(context, instance, disk_info['mapping'],
                            injection_info=injection_info, suffix='.rescue',
                            disk_images=rescue_images)
@@ -4664,7 +4671,7 @@ class LibvirtDriver(driver.ComputeDriver):
                                   image_meta, rescue=rescue_images,
                                   mdevs=mdevs,
                                   block_device_info=block_device_info,
-                                  share_info=share_info)
+                                  share_info=share_info, old_guest=old_guest)
         self._destroy(instance)
         self._create_guest(
             context, xml, instance, post_xml_callback=gen_confdrive,
@@ -7123,12 +7130,55 @@ class LibvirtDriver(driver.ComputeDriver):
 
         return supported_events
 
+    def _copy_guest_firmware_elements(
+        self,
+        old_guest: vconfig.LibvirtConfigGuest,
+        guest: vconfig.LibvirtConfigGuest,
+    ) -> None:
+        loader = old_guest.os_loader
+        nvram_template = old_guest.os_nvram_template
+
+        if guest.os_loader_secure != old_guest.os_loader_secure:
+            LOG.warning('Secure boot support was changed '
+                        'after this instance had been created. '
+                        'Re-selecting the firmware files.')
+            # TODO(tkajinam): VIR_DOMAIN_START_RESET_NVRAM should
+            # be added to guest.start if the hard_reboot method is modified
+            # so that it keeps NVRAM file.
+        elif loader and not os.path.exists(loader):
+            # NOTE(tkajinam): Loader does not exist in this host
+            LOG.debug('The previous loader file %s does not '
+                      'exist. Force re-selection of firmware.',
+                      loader)
+        elif nvram_template and not os.path.exists(nvram_template):
+            LOG.debug('The previous nvram template file %s does '
+                      'not exist. Force re-selection of firmware.',
+                      nvram_template)
+        else:
+            # Disable firmware re-selection
+            guest.os_firmware = None
+
+            guest.os_loader = old_guest.os_loader
+            guest.os_loader_type = old_guest.os_loader_type
+            guest.os_loader_readonly = \
+                old_guest.os_loader_readonly
+            guest.os_nvram = old_guest.os_nvram
+            guest.os_nvram_template = old_guest.os_nvram_template
+
+            # if the feature set says we need SMM then enable it
+            for f in old_guest.features:
+                if f == vconfig.LibvirtConfigGuestFeatureSMM():
+                    guest.features.append(
+                        vconfig.LibvirtConfigGuestFeatureSMM())
+                    break
+
     def _configure_guest_by_virt_type(
         self,
         guest: vconfig.LibvirtConfigGuest,
         instance: 'objects.Instance',
         image_meta: 'objects.ImageMeta',
         flavor: 'objects.Flavor',
+        old_guest: ty.Optional[vconfig.LibvirtConfigGuest] = None,
     ) -> None:
         if CONF.libvirt.virt_type in ("kvm", "qemu"):
             caps = self._host.get_capabilities()
@@ -7194,30 +7244,17 @@ class LibvirtDriver(driver.ComputeDriver):
                 else:
                     guest.os_loader_secure = False
 
-                try:
+                guest.os_firmware = 'efi'
-                    loader, nvram_template, requires_smm = (
-                    self._host.get_loader(
-                        arch, mach_type,
-                        has_secure_boot=guest.os_loader_secure))
-                except exception.UEFINotSupported as exc:
-                    if guest.os_loader_secure:
-                        # we raise a specific exception if we requested secure
-                        # boot and couldn't get that
-                        raise exception.SecureBootNotSupported() from exc
-                    raise
-
-                guest.os_loader = loader
-                guest.os_loader_type = 'pflash'
-                guest.os_loader_readonly = True
                 if hw_firmware_stateless:
                     guest.os_loader_stateless = True
-                else:
-                    guest.os_nvram_template = nvram_template
 
-                # if the feature set says we need SMM then enable it
+                if old_guest:
-                if requires_smm:
+                    LOG.debug('The domain already exists. Loading '
-                    guest.features.append(
+                              'the firmware files previously selected.')
-                        vconfig.LibvirtConfigGuestFeatureSMM())
+                    self._copy_guest_firmware_elements(old_guest, guest)
+                else:
+                    LOG.debug('The domain does not exist. Firmware files '
+                              'will be selected by libvirt.')
 
             # NOTE(lyarwood): If the machine type isn't recorded in the stashed
             # image metadata then record it through the system metadata table.
@@ -7549,7 +7586,7 @@ class LibvirtDriver(driver.ComputeDriver):
     def _get_guest_config(self, instance, network_info, image_meta,
                           disk_info, rescue=None, block_device_info=None,
                           context=None, mdevs=None, accel_info=None,
-                          share_info=None):
+                          share_info=None, old_guest=None):
         """Get config data for parameters.
 
         :param rescue: optional dictionary that should contain the key
@@ -7618,7 +7655,8 @@ class LibvirtDriver(driver.ComputeDriver):
 
         me_config = self._get_mem_encryption_config(flavor, image_meta)
 
-        self._configure_guest_by_virt_type(guest, instance, image_meta, flavor)
+        self._configure_guest_by_virt_type(
+            guest, instance, image_meta, flavor, old_guest)
         if CONF.libvirt.virt_type != 'lxc':
             self._conf_non_lxc(
                 guest, root_device_name, rescue, instance, inst_path,
@@ -8111,11 +8149,26 @@ class LibvirtDriver(driver.ComputeDriver):
         ioapic = vconfig.LibvirtConfigGuestFeatureIOAPIC()
         guest.add_feature(ioapic)
 
+    def _get_existing_guest_config(
+        self,
+        instance: 'objects.Instance',
+    ) -> ty.Optional[vconfig.LibvirtConfigGuest]:
+        guest_config = None
+        try:
+            guest = self._host.get_guest(instance)
+            xml = guest.get_xml_desc()
+            xml_doc = etree.fromstring(xml)
+            guest_config = vconfig.LibvirtConfigGuest()
+            guest_config.parse_dom(xml_doc)
+        except exception.InstanceNotFound:
+            pass
+        return guest_config
+
     def _get_guest_xml(self, context, instance, network_info, disk_info,
                        image_meta, rescue=None,
                        block_device_info=None,
                        mdevs=None, accel_info=None,
-                       share_info=None):
+                       share_info=None, old_guest=None):
         # NOTE(danms): Stringifying a NetworkInfo will take a lock. Do
         # this ahead of time so that we don't acquire it while also
         # holding the logging lock.
@@ -8135,7 +8188,8 @@ class LibvirtDriver(driver.ComputeDriver):
         LOG.debug(strutils.mask_password(msg), instance=instance)
         conf = self._get_guest_config(instance, network_info, image_meta,
                                       disk_info, rescue, block_device_info,
-                                      context, mdevs, accel_info, share_info)
+                                      context, mdevs, accel_info, share_info,
+                                      old_guest)
         xml = conf.to_xml()
 
         LOG.debug('End _get_guest_xml xml=%(xml)s',
@@ -12944,10 +12998,12 @@ class LibvirtDriver(driver.ComputeDriver):
         # the new XML
         mdevs = list(self._get_all_assigned_mediated_devices(instance))
 
+        old_guest = self._get_existing_guest_config(instance)
+
         xml = self._get_guest_xml(context, instance, network_info, disk_info,
                                   instance.image_meta,
                                   block_device_info=block_device_info,
-                                  mdevs=mdevs)
+                                  mdevs=mdevs, old_guest=old_guest)
         self._create_guest_with_network(
             context, xml, instance, network_info, block_device_info,
             power_on=power_on)

nova/virt/libvirt/host.py

@@ -30,8 +30,6 @@ the other libvirt related classes
 from collections.abc import Callable
 from collections.abc import Mapping
 from collections import defaultdict
-import fnmatch
-import glob
 import inspect
 import operator
 import os
@@ -41,7 +39,6 @@ import typing as ty
 
 from lxml import etree
 from oslo_log import log as logging
-from oslo_serialization import jsonutils
 from oslo_utils import excutils
 from oslo_utils import strutils
 from oslo_utils import units
@@ -81,39 +78,9 @@ HV_DRIVER_QEMU = "QEMU"
 
 SEV_KERNEL_PARAM_FILE = '/sys/module/kvm_amd/parameters/%s'
 
-# These are taken from the spec
-# https://github.com/qemu/qemu/blob/v5.2.0/docs/interop/firmware.json
-QEMU_FIRMWARE_DESCRIPTOR_PATHS = [
-    '/usr/share/qemu/firmware',
-    '/etc/qemu/firmware',
-    # we intentionally ignore '$XDG_CONFIG_HOME/qemu/firmware'
-]
-
 MIN_QEMU_SEV_ES_VERSION = (8, 0, 0)
 
 
-def _get_loaders():
-    if not any(
-        os.path.exists(path) for path in QEMU_FIRMWARE_DESCRIPTOR_PATHS
-    ):
-        msg = _("Failed to locate firmware descriptor files")
-        raise exception.InternalError(msg)
-
-    _loaders = []
-
-    for path in QEMU_FIRMWARE_DESCRIPTOR_PATHS:
-        if not os.path.exists(path):
-            continue
-
-        for spec_path in sorted(glob.glob(f'{path}/*.json')):
-            with open(spec_path, 'rb') as fh:
-                spec = jsonutils.load(fh)
-
-            _loaders.append(spec)
-
-    return _loaders
-
-
 class LibvirtEventHandler:
     def __init__(self, conn_event_handler=None, lifecycle_event_handler=None):
         self._lifecycle_event_handler = lifecycle_event_handler
@@ -352,8 +319,6 @@ class Host(object):
         self._libvirt_proxy_classes = self._get_libvirt_proxy_classes(libvirt)
         self._libvirt_proxy = self._wrap_libvirt_proxy(libvirt)
 
-        self._loaders: list[dict] | None = None
-
         # A number of features are conditional on support in the hardware,
         # kernel, QEMU, and/or libvirt. These are determined on demand and
         # memoized by various properties below
@@ -2188,75 +2153,3 @@ class Host(object):
         is meant to be checked elsewhere.
         """
         return self.has_min_version(lv_ver=(7, 9, 0))
-
-    @property
-    def loaders(self) -> list[dict]:
-        """Retrieve details of loader configuration for the host.
-
-        Inspect the firmware metadata files provided by QEMU [1] to retrieve
-        information about the firmware supported by this host. Note that most
-        distros only publish this information for UEFI loaders currently.
-
-        This should be removed when libvirt correctly supports switching
-        between loaders with or without secure boot enabled [2].
-
-        [1] https://github.com/qemu/qemu/blob/v5.2.0/docs/interop/firmware.json
-        [2] https://bugzilla.redhat.com/show_bug.cgi?id=1906500
-
-        :returns: An ordered list of loader configuration dictionaries.
-        """
-        if self._loaders is not None:
-            return self._loaders
-
-        self._loaders = _get_loaders()
-        return self._loaders
-
-    def get_loader(
-        self,
-        arch: str,
-        machine: str,
-        has_secure_boot: bool,
-    ) -> tuple[str, str, bool]:
-        """Get loader for the specified architecture and machine type.
-
-        :returns: A the bootloader executable path and the NVRAM
-            template path and a bool indicating if we need to enable SMM.
-        """
-
-        machine = self.get_canonical_machine_type(arch, machine)
-
-        for loader in self.loaders:
-            try:
-                for target in loader['targets']:
-                    if arch != target['architecture']:
-                        continue
-
-                    for machine_glob in target['machines']:
-                        # the 'machines' attribute supports glob patterns (e.g.
-                        # 'pc-q35-*') so we need to resolve these
-                        if fnmatch.fnmatch(machine, machine_glob):
-                            break
-                    else:
-                        continue
-
-                    # if we've got this far, we have a match on the target
-                    break
-                else:
-                    continue
-
-                # if we request secure boot then we should get it and vice
-                # versa
-                if has_secure_boot != ('secure-boot' in loader['features']):
-                    continue
-
-                return (
-                    loader['mapping']['executable']['filename'],
-                    loader['mapping']['nvram-template']['filename'],
-                    'requires-smm' in loader['features'],
-                )
-            except KeyError:
-                # This indicates that the description structure is new and nova
-                # does not how to handle it
-                continue
-
-        raise exception.UEFINotSupported()

releasenotes/notes/bp-libvirt-firmware-auto-selection-2d58b075816898b7.yaml

@@ -0,0 +1,16 @@
+---
+features:
+  - |
+    Now the libvirt virt driver uses firmware auto-selection by libvirt, which
+    is capable to select the appropriate firmware files according to
+    the requested features. This built-in auto-selection extends the existing
+    firmware selection capability within nova, and checks a few more feature
+    flags such as amd-sev and also supports new firmware types such as rom
+    firmware.
+
+upgrades:
+  - |
+    Existing UEFI guests may start using a new firmware file after operations
+    like rebuild or resize (including cold-migration) which generates libvirt
+    domain XML from scratch, due to the extended logic to select
+    the appropriate files.