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

[Philipp Giersfeld <philipp.giersfeld@canarybit.eu>]

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?

> 
> > +			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.

> 
> > +	],
> > +	'4m-snp' => [
> > +	    "$EDK2_FW_BASE/OVMF_CVM_4M.fd",
> > +	],
> >  	# FIXME: These are legacy 2MB-sized images that modern OVMF doesn't supports to build
> >  	# anymore. how can we deperacate this sanely without breaking existing instances, or using
> >  	# older backups and snapshot?
> > @@ -3172,15 +3179,22 @@ sub vga_conf_has_spice {
> >      return $1 || 1;
> >  }
> >  
> > -sub get_ovmf_files($$$) {
> > -    my ($arch, $efidisk, $smm) = @_;
> > +sub get_ovmf_files($$$$) {
> > +    my ($arch, $efidisk, $smm, $amd_sev_type) = @_;
> >  
> >      my $types = $OVMF->{$arch}
> >  	or die "no OVMF images known for architecture '$arch'\n";
> >  
> >      my $type = 'default';
> >      if ($arch eq 'x86_64') {
> > -	if (defined($efidisk->{efitype}) && $efidisk->{efitype} eq '4m') {
> > +	if ($amd_sev_type && $amd_sev_type eq 'snp') {
> > +	    $type = "4m-snp";
> > +	    my ($ovmf) = $types->{$type}->@*;
> > +	    die "EFI base image '$ovmf' not found\n" if ! -f $ovmf;
> > +	    return ($ovmf);
> > +	} elsif ($amd_sev_type) {
> > +	    $type = "4m-sev";
> > +	} elsif (defined($efidisk->{efitype}) && $efidisk->{efitype} eq '4m') {
> >  	    $type = $smm ? "4m" : "4m-no-smm";
> >  	    $type .= '-ms' if $efidisk->{'pre-enrolled-keys'};
> >  	} else {
> > @@ -3329,7 +3343,8 @@ my sub print_ovmf_drive_commandlines {
> >  
> >      my $d = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
> >  
> > -    my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
> > +    my $amd_sev_type = get_amd_sev_type($conf);
> 
> I'd die here if the type is 'snp' just to be sure, so that we have a
> clear error should that ever happen by accident.

Ack

> 
> > +    my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35, $amd_sev_type);
> >  
> >      my $var_drive_str = "if=pflash,unit=1,id=drive-efidisk0";
> >      if ($d) {
> > @@ -3526,10 +3541,17 @@ sub config_to_command {
> >  	die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n"
> >  	    if !$forcecpu && get_cpu_bitness($conf->{cpu}, $arch) == 32;
> >  
> > -	my ($code_drive_str, $var_drive_str) =
> > -	    print_ovmf_drive_commandlines($conf, $storecfg, $vmid, $arch, $q35, $version_guard);
> > -	push $cmd->@*, '-drive', $code_drive_str;
> > -	push $cmd->@*, '-drive', $var_drive_str;
> > +	my $amd_sev_type = get_amd_sev_type($conf);
> > +	if ($amd_sev_type && $amd_sev_type eq 'snp') {
> > +	   my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
> > +	   my $d = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
> 
> Since the EFI disk is not used in this case, we should always pass undef
> to be explicit. We can print an informational message that it will be
> ignored if present.
> 
> > +	   push $cmd->@*, '-bios', get_ovmf_files($arch, $d, undef, $amd_sev_type);
> > +	} else {
> > +	    my ($code_drive_str, $var_drive_str) = print_ovmf_drive_commandlines(
> > +		$conf, $storecfg, $vmid, $arch, $q35, $version_guard);
> > +	    push $cmd->@*, '-drive', $code_drive_str;
> > +	    push $cmd->@*, '-drive', $var_drive_str;
> > +	}
> >      }
> >  
> >      if ($q35) { # tell QEMU to load q35 config early
> 
> ---snip 8<---
> 
> > @@ -231,25 +232,32 @@ my $cpu_fmt = {
> >  my $sev_fmt = {
> >      type => {
> >  	description => "Enable standard SEV with type='std' or enable"
> > -	    ." experimental SEV-ES with the 'es' option.",
> > +	    ." experimental SEV-ES with the 'es' option or enable "
> > +	    ."experimental SEV-SNP with the 'snp' option.",
> 
> Style nit: it's preferred to use the space at the beginning of the next
> line: https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Strings
> 
Ack.
> >  	type => 'string',
> >  	default_key => 1,
> >  	format_description => "sev-type",
> > -	enum => ['std', 'es'],
> > +	enum => ['std', 'es', 'snp'],
> >  	maxLength => 3,
> >      },
> >      'no-debug' => {
> > -	description => "Sets policy bit 0 to 1 to disallow debugging of guest",
> > +	description => "Sets policy bit to disallow debugging of guest",
> >  	type => 'boolean',
> >  	default => 0,
> >  	optional => 1,
> >      },
> >      'no-key-sharing' => {
> > -	description => "Sets policy bit 1 to 1 to disallow key sharing with other guests",
> > +	description => "Sets policy bit to disallow key sharing with other guests",
> 
> I'd also mention that it is ignored if using SNP
> 
Ack
> >  	type => 'boolean',
> >  	default => 0,
> >  	optional => 1,
> >      },
> > +    'allow-smt' => {
> > +	description => "Sets policy bit to allow Simultaneous Multi Threading (SMT)",
> 
> I'd also mention that it is ignored if not using SNP
> 
Ack
> > +	type => 'boolean',
> > +	default => 1,
> > +	optional => 1,
> > +    },
> >      "kernel-hashes" => {
> >  	description => "Add kernel hashes to guest firmware for measured linux kernel launch",
> >  	type => 'boolean',
> > @@ -823,6 +831,13 @@ sub get_hw_capabilities {
> >      }
> >      return $hw_capabilities;
> >  }
> > +sub get_amd_sev_type {
> > +    my ($conf) = @_;
> > +
> > +    return undef if !$conf->{'amd-sev'};
> > +
> > +    return PVE::JSONSchema::parse_property_string($sev_fmt, $conf->{'amd-sev'})->{type};
> > +}
> >  
> >  sub get_amd_sev_object {
> >      my ($amd_sev, $bios) = @_;
> > @@ -836,22 +851,41 @@ sub get_amd_sev_object {
> >      if ($amd_sev_conf->{type} eq 'es' && !$sev_hw_caps->{'sev-support-es'}) {
> >  	die "Your CPU does not support AMD SEV-ES.\n";
> >      }
> > +    if ($amd_sev_conf->{type} eq 'snp' && !$sev_hw_caps->{'sev-support-snp'}) {
> > +	die "Your CPU does not support AMD SEV-SNP.\n";
> > +    }
> >      if (!$bios || $bios ne 'ovmf') {
> >  	die "To use AMD SEV, you need to change the BIOS to OVMF.\n";
> >      }
> >  
> > -    my $sev_mem_object = 'sev-guest,id=sev0';
> > -    $sev_mem_object .= ',cbitpos='.$sev_hw_caps->{cbitpos};
> > -    $sev_mem_object .= ',reduced-phys-bits='.$sev_hw_caps->{'reduced-phys-bits'};
> > -
> > -    # guest policy bit calculation as described here:
> > -    # https://documentation.suse.com/sles/15-SP5/html/SLES-amd-sev/article-amd-sev.html#table-guestpolicy
> > -    my $policy = 0;
> > -    $policy |= 1 << 0 if $amd_sev_conf->{'no-debug'};
> > -    $policy |= 1 << 1 if $amd_sev_conf->{'no-key-sharing'};
> > -    $policy |= 1 << 2 if $amd_sev_conf->{type} eq 'es';
> > -    # disable migration with bit 3 nosend to prevent amd-sev-migration-attack
> > -    $policy |= 1 << 3;
> > +    my $sev_mem_object = '';
> > +    my $policy;
> > +    if ($amd_sev_conf->{type} eq 'es' || $amd_sev_conf->{type} eq 'std') {
> > +	$sev_mem_object .= 'sev-guest,id=sev0';
> > +	$sev_mem_object .= ',cbitpos='.$sev_hw_caps->{cbitpos};
> > +	$sev_mem_object .= ',reduced-phys-bits='.$sev_hw_caps->{'reduced-phys-bits'};
> > +
> > +	# guest policy bit calculation as described here:
> > +	# https://documentation.suse.com/sles/15-SP5/html/SLES-amd-sev/article-amd-sev.html#table-guestpolicy
> > +	$policy = 0;
> > +	$policy |= 1 << 0 if $amd_sev_conf->{'no-debug'};
> > +	$policy |= 1 << 1 if $amd_sev_conf->{'no-key-sharing'};
> > +	$policy |= 1 << 2 if $amd_sev_conf->{type} eq 'es';
> > +	# disable migration with bit 3 nosend to prevent amd-sev-migration-attack
> > +	$policy |= 1 << 3;
> > +    } elsif ($amd_sev_conf->{type} eq 'snp') {
> > +	$sev_mem_object .= 'sev-snp-guest,id=sev0';
> > +	$sev_mem_object .= ',cbitpos='.$sev_hw_caps->{cbitpos};
> > +	$sev_mem_object .= ',reduced-phys-bits='.$sev_hw_caps->{'reduced-phys-bits'};
> > +
> > +	# guest policy bit calculation as described in chapter 4.3:
> > +	# https://www.amd.com/system/files/TechDocs/56860.pdf
> > +	# Reserved bit must be one
> > +	$policy = 1 << 17;
> > +	$policy |= 1 << 16 if $amd_sev_conf->{'allow-smt'};
> 
> In the schema, the default is set to 1. Note that this is only
> informational unfortunately, i.e. parse_property_string() will not fill
> in a default. You need to apply the default yourself here using
> 
> > if !defined($amd_sev_conf->{'allow-smt'}) || $amd_sev_conf->{'allow-smt'};
> 
Ack
> 
> > +	$policy |= 1 << 19 if !$amd_sev_conf->{'no-debug'};
> 
> Note that here, it already works automatically, because the default is 0.
> 
> > +    }
> > +
> >  
> >      $sev_mem_object .= ',policy='.sprintf("%#x", $policy);
> >      $sev_mem_object .= ',kernel-hashes=on' if ($amd_sev_conf->{'kernel-hashes'});
> 
> 

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