[PATCH v2 04/10] tests: Add tests for SEV firmware selection

Andrea Bolognani via Devel posted 10 patches 1 week, 5 days ago
[PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Andrea Bolognani via Devel 1 week, 5 days ago
One of the new test cases demonstrates how firmware
autoselection doesn't currently work correctly for domains
using SEV-SNP: the descriptor for a suitable firmware exists,
and yet it doesn't get picked up.

The other test cases shows that, while firmware autoselection
succeeds for non-SNP SEV domains, the results are not the
expected ones: the generic (stateful) edk2 build is used
instead of the SEV-specific (stateless) one.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 ...-auto-efi-sev-snp.x86_64-latest+amdsev.err |  1 +
 ...-auto-efi-sev-snp.x86_64-latest+amdsev.xml | 38 ++++++++++++++++
 .../firmware-auto-efi-sev-snp.xml             | 20 +++++++++
 ...are-auto-efi-sev.x86_64-latest+amdsev.args | 37 ++++++++++++++++
 ...ware-auto-efi-sev.x86_64-latest+amdsev.xml | 43 +++++++++++++++++++
 .../qemuxmlconfdata/firmware-auto-efi-sev.xml | 20 +++++++++
 tests/qemuxmlconftest.c                       |  8 ++++
 7 files changed, 167 insertions(+)
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.err
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.xml
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.xml
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.xml
 create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev.xml

diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.err b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.err
new file mode 100644
index 0000000000..3edb2b3451
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.err
@@ -0,0 +1 @@
+operation failed: Unable to find 'efi' firmware that is compatible with the current configuration
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.xml b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.xml
new file mode 100644
index 0000000000..81ac7888ea
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.xml
@@ -0,0 +1,38 @@
+<domain type='kvm'>
+  <name>guest</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <currentMemory unit='KiB'>1048576</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-10.0'>hvm</type>
+    <loader format='raw'/>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <cpu mode='custom' match='exact' check='none'>
+    <model fallback='forbid'>qemu64</model>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0' model='none'/>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pcie-root'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <audio id='1' type='none'/>
+    <watchdog model='itco' action='reset'/>
+    <memballoon model='none'/>
+  </devices>
+  <launchSecurity type='sev-snp'>
+    <policy>0x00030000</policy>
+  </launchSecurity>
+</domain>
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.xml b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.xml
new file mode 100644
index 0000000000..4bb363d07a
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.xml
@@ -0,0 +1,20 @@
+<domain type='kvm'>
+  <name>guest</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-10.0'>hvm</type>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' model='none'/>
+    <memballoon model='none'/>
+  </devices>
+  <launchSecurity type='sev-snp'>
+    <policy>0x30000</policy>
+  </launchSecurity>
+</domain>
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
new file mode 100644
index 0000000000..550ac52b8a
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
@@ -0,0 +1,37 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
+-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
+-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \
+-machine pc-q35-10.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,confidential-guest-support=lsec0,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,acpi=on \
+-accel kvm \
+-cpu qemu64 \
+-m size=1048576k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-global ICH9-LPC.noreboot=off \
+-watchdog-action reset \
+-object '{"qom-type":"sev-guest","id":"lsec0","cbitpos":51,"reduced-phys-bits":1,"policy":196608}' \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.xml b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.xml
new file mode 100644
index 0000000000..cbfdcdeee3
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.xml
@@ -0,0 +1,43 @@
+<domain type='kvm'>
+  <name>guest</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <currentMemory unit='KiB'>1048576</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-10.0'>hvm</type>
+    <firmware>
+      <feature enabled='no' name='enrolled-keys'/>
+      <feature enabled='no' name='secure-boot'/>
+    </firmware>
+    <loader readonly='yes' type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE.fd</loader>
+    <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.fd' templateFormat='raw' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <cpu mode='custom' match='exact' check='none'>
+    <model fallback='forbid'>qemu64</model>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0' model='none'/>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pcie-root'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <audio id='1' type='none'/>
+    <watchdog model='itco' action='reset'/>
+    <memballoon model='none'/>
+  </devices>
+  <launchSecurity type='sev'>
+    <policy>0x30000</policy>
+  </launchSecurity>
+</domain>
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev.xml b/tests/qemuxmlconfdata/firmware-auto-efi-sev.xml
new file mode 100644
index 0000000000..69e0c2bd51
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev.xml
@@ -0,0 +1,20 @@
+<domain type='kvm'>
+  <name>guest</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-10.0'>hvm</type>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' model='none'/>
+    <memballoon model='none'/>
+  </devices>
+  <launchSecurity type='sev'>
+    <policy>0x30000</policy>
+  </launchSecurity>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 079f20ddf4..c17bb168f6 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1477,6 +1477,14 @@ mymain(void)
     DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-format-loader-raw", "aarch64");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch");
 
+    DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-auto-efi-sev", "x86_64",
+                                  ARG_CAPS_VARIANT, "+amdsev",
+                                  ARG_END);
+    DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-auto-efi-sev-snp", "x86_64",
+                                  ARG_FLAGS, FLAG_EXPECT_FAILURE,
+                                  ARG_CAPS_VARIANT, "+amdsev",
+                                  ARG_END);
+
     DO_TEST_CAPS_LATEST("clock-utc");
     DO_TEST_CAPS_LATEST("clock-localtime");
     DO_TEST_CAPS_LATEST("clock-localtime-basis-localtime");
-- 
2.51.0
Re: [PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Jim Fehlig via Devel 1 week, 5 days ago
On 8/25/25 10:19, Andrea Bolognani via Devel wrote:
> One of the new test cases demonstrates how firmware
> autoselection doesn't currently work correctly for domains
> using SEV-SNP: the descriptor for a suitable firmware exists,
> and yet it doesn't get picked up.

But the descriptor is incorrect. Autoselection using current git master works 
fine with a proper descriptor for SNP.

IMO, we need to fix the descriptors (patches 8 and 9) before adding more tests 
with invalid config.

> The other test cases shows that, while firmware autoselection
> succeeds for non-SNP SEV domains, the results are not the
> expected ones: the generic (stateful) edk2 build is used
> instead of the SEV-specific (stateless) one.

We need patch 9 to prevent selection of the stateful firmware, but then we'd hit 
the problem fixed by patch 5 :-).

...

> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
> new file mode 100644
> index 0000000000..550ac52b8a
> --- /dev/null
> +++ b/tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
> @@ -0,0 +1,37 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/var/lib/libvirt/qemu/domain--1-guest \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
> +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
> +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=guest,debug-threads=on \
> +-S \
> +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
> +-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
> +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
> +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \

Writable pflash is not compatible with SEV(-ES) guests.

Regards,
Jim
Re: [PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Andrea Bolognani via Devel 1 week, 4 days ago
On Mon, Aug 25, 2025 at 05:12:57PM -0600, Jim Fehlig wrote:
> On 8/25/25 10:19, Andrea Bolognani via Devel wrote:
> > One of the new test cases demonstrates how firmware
> > autoselection doesn't currently work correctly for domains
> > using SEV-SNP: the descriptor for a suitable firmware exists,
> > and yet it doesn't get picked up.
>
> But the descriptor is incorrect. Autoselection using current git master
> works fine with a proper descriptor for SNP.

It's true, the current descriptor for SEV-SNP is incorrect as it
causes libvirt to use pflash instead of rom. But the fact that
libvirt will ignore the current descriptor unless

  <loader stateless='yes'/>

is present in the domain configuration, as demonstrated by the test
case that I'm adding in this patch, is a problem of its own, and
indeed the one that you reported in the first place ;)

So yes, we need to fix both issues, the one in libvirt and the one in
the descriptors. Solving the latter first would merely sweep the
former under the carpet, not make it go away.

> IMO, we need to fix the descriptors (patches 8 and 9) before adding more
> tests with invalid config.

I'm doing things in this order deliberately. Adding a failing test
establishes the current baseline for the functionality, so that when
the fix is applied you can see the improvement reflected directly in
the test suite, confirming its effectiveness. Adding tests after the
fact only demonstrates that the feature now works, not that it was
broken beforehand.

> > +-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
> > +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
> > +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \
>
> Writable pflash is not compatible with SEV(-ES) guests.

Is that so? According to

  https://libvirt.org/kbase/launch_security_sev.html

a stateless firmware is only a requirement if boot measurements are
desired, which IIUC is not necessarily always the case.

In fact, the full XML example at the bottom of that document is using
stateful firmware.

To be clear, I'm tentatively in favor of moving towards a world in
which stateless firmware is used consistently across the board for
SEV guests, but we need to ensure that we don't cause disruption for
existing users in the process.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Jim Fehlig via Devel 1 week, 4 days ago
On 8/26/25 09:30, Andrea Bolognani wrote:
> On Mon, Aug 25, 2025 at 05:12:57PM -0600, Jim Fehlig wrote:
>> On 8/25/25 10:19, Andrea Bolognani via Devel wrote:
>>> One of the new test cases demonstrates how firmware
>>> autoselection doesn't currently work correctly for domains
>>> using SEV-SNP: the descriptor for a suitable firmware exists,
>>> and yet it doesn't get picked up.
>>
>> But the descriptor is incorrect. Autoselection using current git master
>> works fine with a proper descriptor for SNP.
> 
> It's true, the current descriptor for SEV-SNP is incorrect as it
> causes libvirt to use pflash instead of rom. But the fact that
> libvirt will ignore the current descriptor unless
> 
>    <loader stateless='yes'/>
> 
> is present in the domain configuration, as demonstrated by the test
> case that I'm adding in this patch, is a problem of its own, and
> indeed the one that you reported in the first place ;)

Yep, no arguing that point.

> So yes, we need to fix both issues, the one in libvirt and the one in
> the descriptors. Solving the latter first would merely sweep the
> former under the carpet, not make it go away.

I think the same could be said by fixing libvirt first.

> 
>> IMO, we need to fix the descriptors (patches 8 and 9) before adding more
>> tests with invalid config.
> 
> I'm doing things in this order deliberately. Adding a failing test
> establishes the current baseline for the functionality, so that when
> the fix is applied you can see the improvement reflected directly in
> the test suite, confirming its effectiveness. Adding tests after the
> fact only demonstrates that the feature now works, not that it was
> broken beforehand.
> 
>>> +-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
>>> +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
>>> +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \
>>
>> Writable pflash is not compatible with SEV(-ES) guests.
> 
> Is that so? According to
> 
>    https://libvirt.org/kbase/launch_security_sev.html
> 
> a stateless firmware is only a requirement if boot measurements are
> desired, which IIUC is not necessarily always the case.

Dammit, you're right. I need to remember some of the SNP/TDX restrictions do not 
apply to SEV(-ES). Too bad we're stuck supporting that transitional technology.

> 
> In fact, the full XML example at the bottom of that document is using
> stateful firmware.
> 
> To be clear, I'm tentatively in favor of moving towards a world in
> which stateless firmware is used consistently across the board for
> SEV guests, but we need to ensure that we don't cause disruption for
> existing users in the process.

Agreed. Changing the actual edk2 descriptors per patch 9 may cause disruptions 
for users wanting a persistent variable store in pflash for their SEV(-ES) guests.

Regards,
Jim
Re: [PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Andrea Bolognani via Devel 1 week, 4 days ago
On Tue, Aug 26, 2025 at 10:39:30AM -0600, Jim Fehlig wrote:
> On 8/26/25 09:30, Andrea Bolognani wrote:
> > On Mon, Aug 25, 2025 at 05:12:57PM -0600, Jim Fehlig wrote:
> > > On 8/25/25 10:19, Andrea Bolognani via Devel wrote:
> > > > One of the new test cases demonstrates how firmware
> > > > autoselection doesn't currently work correctly for domains
> > > > using SEV-SNP: the descriptor for a suitable firmware exists,
> > > > and yet it doesn't get picked up.
> > >
> > > But the descriptor is incorrect. Autoselection using current git master
> > > works fine with a proper descriptor for SNP.
> >
> > It's true, the current descriptor for SEV-SNP is incorrect as it
> > causes libvirt to use pflash instead of rom. But the fact that
> > libvirt will ignore the current descriptor unless
> >
> >    <loader stateless='yes'/>
> >
> > is present in the domain configuration, as demonstrated by the test
> > case that I'm adding in this patch, is a problem of its own, and
> > indeed the one that you reported in the first place ;)
>
> Yep, no arguing that point.
>
> > So yes, we need to fix both issues, the one in libvirt and the one in
> > the descriptors. Solving the latter first would merely sweep the
> > former under the carpet, not make it go away.
>
> I think the same could be said by fixing libvirt first.

Not really, because after you've fixed libvirt (patch 05/10) the test
case lands in an "intermediate" state where autoselection succeeds,
but you still get the wrong mode being used (pflash instead of ROM).
Only after you fix the descriptors too (patch 08/10) you end up with
the desired state. Doing things in this order gives you the full
progression of the fix clearly visible in the git log.

> > To be clear, I'm tentatively in favor of moving towards a world in
> > which stateless firmware is used consistently across the board for
> > SEV guests, but we need to ensure that we don't cause disruption for
> > existing users in the process.
>
> Agreed. Changing the actual edk2 descriptors per patch 9 may cause
> disruptions for users wanting a persistent variable store in pflash for
> their SEV(-ES) guests.

Maybe we can cater to that use case by adding a low-priority
descriptor that is identical to the regular edk2 one but advertises
amd-sev and amd-sev-es features, and by asking users that want a
persistent variable store to ask for

  <loader stateless='no'/>

instead? Assuming people looking to stateful SEV are a small
minority, this sounds like it might be feasible. And then we could
use the ROM loader for everyone else. We wouldn't even need separate
descriptors for SEV-SNP and SEV(-ES), just for stateless SEV and
stateful SEV.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/10] tests: Add tests for SEV firmware selection
Posted by Jim Fehlig via Devel 1 week, 3 days ago
On 8/26/25 11:12, Andrea Bolognani wrote:
> On Tue, Aug 26, 2025 at 10:39:30AM -0600, Jim Fehlig wrote:
>> On 8/26/25 09:30, Andrea Bolognani wrote:
>>> On Mon, Aug 25, 2025 at 05:12:57PM -0600, Jim Fehlig wrote:
>>>> On 8/25/25 10:19, Andrea Bolognani via Devel wrote:
>>>>> One of the new test cases demonstrates how firmware
>>>>> autoselection doesn't currently work correctly for domains
>>>>> using SEV-SNP: the descriptor for a suitable firmware exists,
>>>>> and yet it doesn't get picked up.
>>>>
>>>> But the descriptor is incorrect. Autoselection using current git master
>>>> works fine with a proper descriptor for SNP.
>>>
>>> It's true, the current descriptor for SEV-SNP is incorrect as it
>>> causes libvirt to use pflash instead of rom. But the fact that
>>> libvirt will ignore the current descriptor unless
>>>
>>>     <loader stateless='yes'/>
>>>
>>> is present in the domain configuration, as demonstrated by the test
>>> case that I'm adding in this patch, is a problem of its own, and
>>> indeed the one that you reported in the first place ;)
>>
>> Yep, no arguing that point.
>>
>>> So yes, we need to fix both issues, the one in libvirt and the one in
>>> the descriptors. Solving the latter first would merely sweep the
>>> former under the carpet, not make it go away.
>>
>> I think the same could be said by fixing libvirt first.
> 
> Not really, because after you've fixed libvirt (patch 05/10) the test
> case lands in an "intermediate" state where autoselection succeeds,
> but you still get the wrong mode being used (pflash instead of ROM).
> Only after you fix the descriptors too (patch 08/10) you end up with
> the desired state. Doing things in this order gives you the full
> progression of the fix clearly visible in the git log.
> 
>>> To be clear, I'm tentatively in favor of moving towards a world in
>>> which stateless firmware is used consistently across the board for
>>> SEV guests, but we need to ensure that we don't cause disruption for
>>> existing users in the process.
>>
>> Agreed. Changing the actual edk2 descriptors per patch 9 may cause
>> disruptions for users wanting a persistent variable store in pflash for
>> their SEV(-ES) guests.
> 
> Maybe we can cater to that use case by adding a low-priority
> descriptor that is identical to the regular edk2 one but advertises
> amd-sev and amd-sev-es features, and by asking users that want a
> persistent variable store to ask for
> 
>    <loader stateless='no'/>
> 
> instead? Assuming people looking to stateful SEV are a small
> minority, this sounds like it might be feasible. And then we could
> use the ROM loader for everyone else. We wouldn't even need separate
> descriptors for SEV-SNP and SEV(-ES), just for stateless SEV and
> stateful SEV.
> 

This sounds like a viable option to me. Note that only the stateless descriptor 
advertises support for SEV(-ES) in the SUSE edk2 package, so I'm not facing this 
issue downstream.

Regards,
Jim