Re: [pve-devel] [PATCH qemu-server v3 4/5] config: add AMD SEV-SNP support.

[Fiona Ebner <f.ebner@proxmox.com>]

Am 11.03.25 um 14:56 schrieb Philipp Giersfeld:
> On 25/03/05 04:35PM, Fiona Ebner wrote:
>> Am 24.02.25 um 13:37 schrieb Philipp Giersfeld:
>>> This patch is for enabling AMD SEV-SNP support.
>>>
>>> Where applicable, it extends support for existing SEV(-ES) variables
>>> to SEV-SNP. This means that it retains no-debug and kernel-hashes
>>> options, but the no-key-sharing option is removed.
>>>
>>> The default policy value is identical to QEMU’s, and the therefore
>>> required option has been added to configure SMT support.
>>>
>>> The code was tested by running a VM without SEV, with SEV, SEV-ES,
>>> SEV-SNP. Each configuration was tested with and without an EFI disk
>>> attached. For SEV-enabled configurations it was also verified that the
>>> kernel actually used the respective feature.
>>>
>>> Signed-off-by: Philipp Giersfeld <philipp.giersfeld@canarybit.eu>
>>> Reviewed-by: Daniel Kral <d.kral@proxmox.com>
>>> Tested-by: Markus Frank <m.frank@proxmox.com>
>>> ---
>>>
>>>  no changes since last version
>>>
>>>  PVE/API2/Qemu.pm            |  7 +++-
>>>  PVE/QemuServer.pm           | 49 +++++++++++++++++++--------
>>>  PVE/QemuServer/CPUConfig.pm | 66 ++++++++++++++++++++++++++++---------
>>>  3 files changed, 92 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>>> index 295260e7..4f48cf85 100644
>>> --- a/PVE/API2/Qemu.pm
>>> +++ b/PVE/API2/Qemu.pm
>>> @@ -548,8 +548,13 @@ my sub create_disks : prototype($$$$$$$$$$$) {
>>>  		my $volid;
>>>  		if ($ds eq 'efidisk0') {
>>>  		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>>> +
>>> +		    my $amd_sev_type = PVE::QemuServer::CPUConfig::get_amd_sev_type($conf);
>>> +		    die "SEV-SNP uses consolidated read-only firmware"
>>
>> Missing newline at the end of error message. Perl will automatically
>> attach the filename and line number then, which is rather ugly here. I'd
>> also mention "and doesn't need an EFI disk" to clarify this a bit more
>> for users.
> 
> Does "SEV-SNP uses consolidated read-only firmware instead of a seperate
> non-volatile variable store\n" work?

I'd rather mention the "EFI disk" directly. Like that it will be clear
to users, since they know the "EFI disk" from the UI/configuration.

> 
>>
>>> +			if $amd_sev_type && $amd_sev_type eq 'snp';
>>> +
>>>  		    ($volid, $size) = PVE::QemuServer::create_efidisk(
>>> -			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
>>> +			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm, $amd_sev_type);
>>>  		} elsif ($ds eq 'tpmstate0') {
>>>  		    # swtpm can only use raw volumes, and uses a fixed size
>>>  		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index 808c0e1c..51e551b8 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -52,7 +52,7 @@ use PVE::QemuConfig;
>>>  use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
>>>  use PVE::QemuServer::Cloudinit;
>>>  use PVE::QemuServer::CGroup;
>>> -use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch get_amd_sev_object);
>>> +use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch get_amd_sev_object get_amd_sev_type);
>>>  use PVE::QemuServer::Drive qw(is_valid_drivename checked_volume_format drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive);
>>>  use PVE::QemuServer::Machine;
>>>  use PVE::QemuServer::Memory qw(get_current_memory);
>>> @@ -88,6 +88,13 @@ my $OVMF = {
>>>  	    "$EDK2_FW_BASE/OVMF_CODE_4M.secboot.fd",
>>>  	    "$EDK2_FW_BASE/OVMF_VARS_4M.ms.fd",
>>>  	],
>>> +	'4m-sev' => [
>>> +	    "$EDK2_FW_BASE/OVMF_CVM_CODE_4M.fd",
>>> +	    "$EDK2_FW_BASE/OVMF_CVM_VARS_4M.fd", 
>>
>> Style nit: trailing whitespace above
>>
>> Is switching over for the std+ES SEV scenarios still
>> required/advantageous if we go for edk2-stable202502?
> 
> From my testing this is still the case.
> IIUC, this is because the existing firmware is built targeting
> OmvfPkg/OvmfPkgI32X64.dsc which does not support SEV.

Okay, thank you for testing!

Best Regards,
Fiona


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel