[pve-devel] [PATCH qemu-server v2 1/2] QEMU AMD SEV enable

[Fiona Ebner]

Am 11.11.22 um 15:27 schrieb Markus Frank:
> This Patch is for enabling AMD SEV (Secure Encrypted
> Virtualization) support in QEMU and enabling future
> memory encryption technologies like INTEL MKTME
> (Multi-key Total Memory Encryption) and SEV-SNP.
> 
> Config-Example:
> memory_encryption: type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1
> 
> reduced-phys-bios, cbitpos and policy correspond to the varibles with the
> same name in qemu.
> 
> reduced-phys-bios and cbitpos are system specific and can be read out
> with QMP. If not set by the user, a dummy-vm gets started to read QMP
> for these variables out and save them to config. Afterwards the dummy-vm gets
> stopped.

Why even allow the user to set them if they are system-specific values?
Or are there multiple possible values on some systems? If not, it should
be a node-specific configuration, rather than a VM-specific one. That
would also only require starting the dummy VM once per node, or we could
require the user to set the values in some node config (of course
mentioning how in the docs :))

> 
> policy can be calculated with the links in comments & description.
> To test SEV-ES (CPU register encryption) the policy should be set
> somewhere between 0x4 and 0x7 or 0xC and 0xF, etc.
> (Bit-2 has to be set 1 (LSB 0 bit numbering))
> 
> SEV needs edk2-OVMF to work.
> 
> Signed-off-by: Markus Frank <m.frank at proxmox.com>
> ---
>  PVE/QemuServer.pm | 133 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 513a248..2ea8abd 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -175,6 +175,58 @@ my $agent_fmt = {
>      },
>  };
>  
> +my $memory_encryption_fmt = {
> +    type => {
> +	type => 'string',
> +	default_key => 1,
> +	description => "Memory Encryption Type:"

Nit: I'd rather have the description be a sentence or two, what it's all
about and add a verbose_description to describe the individual variants.

> +	    ." for AMD SEV -> 'memory_encryption: type=sev';"
> +	    ." for AMD SEV-ES -> use 'sev' and change policy to between 0x4 and 0x7;"
> +	    ." (Bit-2 has to be set 1 (LSB 0 bit numbering))"

Nit: better to use 0x0004 and 0x0007, because 0x4 and 0x7 are not valid
values for 'policy' below.

> +	    #. "for AMD SEV-SNP -> 'memory_encryption: type=sev-snp'"
> +	    ." (sev requires edk2-ovmf & sev kernel support by guest operating system &"
> +	    ." on host: add kernel-parameters 'mem_encrypt=on kvm_amd.sev=1')"
> +	    ." see https://github.com/AMDESE/AMDSEV &"
> +	    ." https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html",
> +	format_description => "qemu-memory-encryption-type",
> +	# TODO enable sev-snp option when feature can be tested on 3rd-gen EPYC
> +	# https://www.phoronix.com/news/AMD-SEV-SNP-Arrives-Linux-5.19
> +	# enum => ['sev','sev-snp','mktme'],

Nit: I feel like these comments don't really belong in the patch. Maybe
just add a single high-level TODO comment? The rest should be done by
the patch actually adding sev-snp ;)

Also, the many links might be better left to the documentation patch.

Is the rest of the format even compatible with Intel's MKTME? I.e.
does/will that also have reduced-phys-bits, 4 policy bits and cbitpos?
If there is some overlap or if we expect to be easily able to translate
certain settings, we can still keep a general memory_encryption_fmt, but
otherwise, it might be better to have completely distinct formats for
Intel and AMD?

> +	enum => ['sev'],
> +	maxLength => 10,
> +    },
> +    'reduced-phys-bits' => {
> +	description => "Number of bits the physical address space is reduced by. System dependent",
> +	type => 'integer',
> +	default => 1,

The default is system-dependent and automatically figured out by the
dummy VM. Also the kvm man pages states

> On EPYC, the value should be 5.

so why 1?

> +	optional => 1,
> +	minimum => 0,
> +	maximum => 100,
> +    },
> +    cbitpos => {
> +	description => "C-bit: marks if a memory page is protected. System dependent",
> +	type => 'integer',
> +	default => 47,

Same here with regards to auto-magic.

> +	optional => 1,
> +	minimum => 0,
> +	maximum => 100,
> +    },
> +    policy => {
> +	description => "SEV Guest Policy"
> +	    . "see Capter 3:"

Nit: typo

> +	    . "https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf"
> +	    . "& https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html",
> +

Could have a verbose_description for each bit rather than the links. Or
should we go one step further and add explicit flags for the
relevant-for-us bits instead? Would be more user-friendly, but the
$memory_encryption_fmt would be AMD-specific of course.

> +	format_description => "qemu-memory-encryption-policy",
> +	type => 'string',
> +	default => '0x0000',
> +	pattern => '0[xX][0-9a-fA-F]{1,4}',
> +	optional => 1,
> +	maxLength => 6,
> +    },
> +};
> +PVE::JSONSchema::register_format('pve-qemu-memory-encryption-fmt', $memory_encryption_fmt);
> +
>  my $vga_fmt = {
>      type => {
>  	description => "Select the VGA type.",
> @@ -349,6 +401,12 @@ my $confdesc = {
>  	minimum => 16,
>  	default => 512,
>      },
> +    memory_encryption => {
> +	description => "Memory Encryption",
> +	optional => 1,
> +	format => 'pve-qemu-memory-encryption-fmt',
> +	type => 'string',
> +    },
>      balloon => {
>  	optional => 1,
>  	type => 'integer',
> @@ -2113,6 +2171,17 @@ sub parse_guest_agent {
>      return $res;
>  }
>  
> +sub parse_memory_encryption {
> +    my ($value) = @_;
> +
> +    return if !$value;
> +
> +    my $res = eval { parse_property_string($memory_encryption_fmt, $value) };
> +    warn $@ if $@;
> +    return $res;
> +}

Why not fail if parsing fails?

> +
> +
>  sub get_qga_key {
>      my ($conf, $key) = @_;
>      return undef if !defined($conf->{agent});
> @@ -4085,6 +4154,70 @@ sub config_to_command {
>      }
>      push @$machineFlags, "type=${machine_type_min}";
>  
> +    # Memory Encryption

Nit: comment contains no additional information.

> +    my $memory_encryption = parse_memory_encryption($conf->{'memory_encryption'});
> +
> +    # Die if bios is not ovmf

Nit: Same here.

> +    if (
> +	$memory_encryption->{'type'}
> +	&& $memory_encryption->{type} eq 'sev'
> +	&& $conf->{bios} ne 'ovmf'

I think $conf->{bios} could be undef here? That would cause an ugly Perl
warning. At least other place check for
    !defined($conf->{bios}) || $conf->{bios} ne 'ovmf'

> +    ) {
> +	die "SEV needs ovmf\n";

Nit: maybe a bit too concise of a message, could mention the settings at
least

> +    }
> +

All of the below should rather live in helper functions, especially the
dummy VM stuff. config_to_command is already much too bloated.

> +    # AMD SEV
> +    if ($memory_encryption->{'type'} && $memory_encryption->{type} =~ /(sev|sev-snp)/) {

Nit: I'd rather have explicit equality testing for the type (or at least
add a leading ^ to the regex). The sev-snp type does not exist yet and
should be added by a later patch.

> +	# Get reduced-phys-bits & cbitpos from QMP, if not set
> +	if (
> +	    !$memory_encryption->{'reduced-phys-bits'}
> +	    || !$memory_encryption->{cbitpos}
> +	) {
> +	    my $fakevmid = -1;
> +	    my $qemu_cmd = get_command_for_arch($arch);
> +	    my $pidfile = PVE::QemuServer::Helpers::pidfile_name($fakevmid);
> +	    my $default_machine = $default_machines->{$arch};
> +	    my $cmd = [
> +		$qemu_cmd,
> +		'-machine', $default_machine,
> +		'-display', 'none',
> +		'-chardev', "socket,id=qmp,path=/var/run/qemu-server/$fakevmid.qmp,server=on,wait=off",
> +		'-mon', 'chardev=qmp,mode=control',
> +		'-pidfile', $pidfile,
> +		'-S', '-daemonize'
> +	    ];

Instead of daemonizing, pidfile etc. we could also use --qmp stdio and
pass the commands via stdin like:
{"execute": "qmp_capabilities"}
{"execute": "query-sev-capabilities"}
{"execute": "quit"}
which might be a bit more straight-forward. But maybe we prefer re-using
the existing infrastructure with the fake ID, not sure?

> +	    my $rc = run_command($cmd, noerr => 1, quiet => 0);
> +	    die "QEMU flag querying VM exited with code " . $rc . "\n" if $rc;
> +	    my $res = mon_cmd($fakevmid, 'query-sev-capabilities');
> +	    $memory_encryption->{'reduced-phys-bits'} = $res->{'reduced-phys-bits'};
> +	    $memory_encryption->{cbitpos} = $res->{cbitpos};
> +	    $conf->{'memory_encryption'} = PVE::JSONSchema::print_property_string(
> +		$memory_encryption,
> +		$memory_encryption_fmt
> +	    );
> +	    PVE::QemuConfig->write_config($vmid, $conf);

config_to_command should not write the config. Hope that those settings
are truly node-specific and not VM-specific :)

> +	    vm_stop(undef, $fakevmid, 1, 1, 10, 0, 1);
> +	}
> +	# qemu-Example: -object 'sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1';
> +	# see https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
> +	my $memobjcmd = "";
> +	if ($memory_encryption->{type} eq 'sev-snp') {
> +	    # future feature: cannot be reached
> +	    $memobjcmd = 'sev-snp-guest';

Nit: should be added by a later patch

> +	} else {
> +	    $memobjcmd = 'sev-guest';
> +	}
> +	$memobjcmd .= ',id=sev0,cbitpos='.$memory_encryption->{cbitpos}
> +	    .',reduced-phys-bits='.$memory_encryption->{'reduced-phys-bits'};
> +	if ($memory_encryption->{type} eq 'sev' && $memory_encryption->{policy}) {
> +	    $memobjcmd .= ',policy='.$memory_encryption->{policy}
> +	}
> +	push @$devices, '-object' , $memobjcmd;
> +	# old qemu-Example: -machine 'type=q35+pve0,memory-encryption=sev0'
> +	# https://fossies.org/linux/qemu/docs/system/confidential-guest-support.rst

Nit: I mean the QEMU docs also mention this so no need for that link and
why the "old" example?

> +	push @$machineFlags, 'confidential-guest-support=sev0';
> +    }
> +
>      push @$cmd, @$devices;
>      push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags);
>      push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags);