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.giersf...@canarybit.eu>
> > Reviewed-by: Daniel Kral <d.k...@proxmox.com>
> > Tested-by: Markus Frank <m.fr...@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
