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

[Markus Frank <m.frank@proxmox.com>]

Thanks for the feedback. I will send v3 when I was able to test it on an EPYC CPU.

On 11/14/22 14:06, Fiona Ebner wrote:
> 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 :))
> 
I moved the system specific parameters to the node config:
amd_sev: cbitpos=47,reduced-phys-bits=1

>>
>> 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@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 ;)
> 
removed
> 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?
Yes. Let's separate 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?

On the EPYC CPUs I have used, the value was 1.
And on https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
they also use reduced-phys-bits=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.
I made everything AMD-specific:

amd_sev: std,es=1,nodbg=1,noks=1

policy will be calculated with these parameters.
Also I would set policy bit 3 (nosend) to 1, because migration
features for sev are not upstream yet and are attackable.

> 
>> +	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?
replaced warn with die
> 
>> +
>> +
>>   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?
What would be the best way to send stdin to "kvm -qmp stdio" here?

Not the same way like I would do in shell or yes?:
echo '{"execute": "qmp_capabilities"} {"execute": "query-sev-capabilities"} {"execute": "quit"}' | kvm  -qmp stdio
> 
>> +	    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
removed
> 
>> +	} 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?
removed
> 
>> +	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);