[pve-devel] [PATCH qemu-server v2 1/2] QEMU AMD SEV enable
Markus Frank
m.frank at proxmox.com
Thu Nov 17 11:50:16 CET 2022
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 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 ;)
>
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);
More information about the pve-devel
mailing list