From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3AAA79189A for ; Mon, 14 Nov 2022 14:06:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 18C38238EE for ; Mon, 14 Nov 2022 14:06:18 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 14 Nov 2022 14:06:15 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E871F43CB4 for ; Mon, 14 Nov 2022 14:06:14 +0100 (CET) Message-ID: <72a2cce6-22c5-52ed-ac5e-8fdebc4beedb@proxmox.com> Date: Mon, 14 Nov 2022 14:06:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Content-Language: en-US To: pve-devel@lists.proxmox.com, m.frank@proxmox.com References: <20221111142716.235955-1-m.frank@proxmox.com> <20221111142716.235955-2-m.frank@proxmox.com> From: Fiona Ebner In-Reply-To: <20221111142716.235955-2-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: =?UTF-8?Q?0=0A=09?=AWL 0.027 Adjusted score from AWL reputation of From: =?UTF-8?Q?address=0A=09?=BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict =?UTF-8?Q?Alignment=0A=09?=KAM_SHORT 0.001 Use of a URL Shortener for very short =?UTF-8?Q?URL=0A=09?=NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF =?UTF-8?Q?Record=0A=09?=SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server v2 1/2] QEMU AMD SEV enable X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2022 13:06:48 -0000 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 > --- > 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);