[pve-devel] [PATCH qemu-server/docs v2 0/2] AMD SEV

[pve-devel] [PATCH qemu-server/docs v2 0/2] AMD SEV

[Markus Frank]

qemu-server:

v2:
* spelling of minimum
* !$conf->{bios} eq 'ovmf' changed to $conf->{bios} ne 'ovmf'

Markus Frank (1):
  QEMU AMD SEV enable

 PVE/QemuServer.pm | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)


docs:

v2:
* added more details for host & clients
* moved things from Limitations to Requirements
* changed order of text

Markus Frank (1):
  added Memory Encryption documentation

 qm.adoc | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

-- 
2.30.2

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

[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.

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:"
+	    ." 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))"
+	    #. "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'],
+	enum => ['sev'],
+	maxLength => 10,
+    },
+    'reduced-phys-bits' => {
+	description => "Number of bits the physical address space is reduced by. System dependent",
+	type => 'integer',
+	default => 1,
+	optional => 1,
+	minimum => 0,
+	maximum => 100,
+    },
+    cbitpos => {
+	description => "C-bit: marks if a memory page is protected. System dependent",
+	type => 'integer',
+	default => 47,
+	optional => 1,
+	minimum => 0,
+	maximum => 100,
+    },
+    policy => {
+	description => "SEV Guest Policy"
+	    . "see Capter 3:"
+	    . "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",
+
+	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;
+}
+
+
 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
+    my $memory_encryption = parse_memory_encryption($conf->{'memory_encryption'});
+
+    # Die if bios is not ovmf
+    if (
+	$memory_encryption->{'type'}
+	&& $memory_encryption->{type} eq 'sev'
+	&& $conf->{bios} ne 'ovmf'
+    ) {
+	die "SEV needs ovmf\n";
+    }
+
+    # AMD SEV
+    if ($memory_encryption->{'type'} && $memory_encryption->{type} =~ /(sev|sev-snp)/) {
+	# 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'
+	    ];
+	    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);
+	    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';
+	} 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
+	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);
-- 
2.30.2

[pve-devel] [PATCH docs v2 2/2] added Memory Encryption documentation

[Markus Frank]

added AMD SEV documentation for "[PATCH qemu-server] QEMU AMD SEV
enable"

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 qm.adoc | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index e7d0c07..5ba43a2 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -598,6 +598,119 @@ systems.
 When allocating RAM to your VMs, a good rule of thumb is always to leave 1GB
 of RAM available to the host.
 
+[[qm_memory_encryption]]
+Memory Encryption
+~~~~~~~~~~~~~~~~~
+
+[[qm_memory_encryption_sev]]
+AMD SEV
+^^^^^^^
+
+Memory Encryption per VM using AES-128 Encryption and the AMD Secure Processor.
+See https://developer.amd.com/sev/[AMD SEV]
+
+*Host-Requirements:*
+
+* AMD EPYC/Ryzen PRO CPU
+* configured SEV BIOS settings on Host Machine
+* add "kvm_amd.sev=1" to kernel parameters if not enabled by default
+* add "mem_encrypt=on" to kernel parameters if you want encrypt memory on the
+host (SME)
+see https://www.kernel.org/doc/Documentation/x86/amd-memory-encryption.txt
+* maybe increase SWIOTLB see https://github.com/AMDESE/AMDSEV#faq-4
+
+To check if SEV is enabled on Host-Machine search for `sev` in dmesg
+and print out the sev kernel parameter of kvm_amd:
+
+----
+# dmesg | grep -i sev
+[...] ccp 0000:45:00.1: sev enabled
+[...] ccp 0000:45:00.1: SEV API: <buildversion>
+[...] SEV supported: <number> ASIDs
+[...] SEV-ES supported: <number> ASIDs
+# cat /sys/module/kvm_amd/parameters/sev
+Y
+----
+
+*Guest-VM-Requirements:*
+
+* edk2-OVMF
+* advisable to use Q35
+* The guest operating system inside the VM must contain SEV-support
+* if there are problems while booting (stops at blank/splash screen or "Guest has not
+initialized the display (yet)") try to add virtio-rng and/or set "freeze: 1"
+so that you wait a few seconds before you click on *Resume* to boot.
+
+*Limitations:*
+
+* Because the memory is encrypted the memory usage on host is always wrong
+* Operations that involve saving or restoring memory like snapshots
+& live migration do not work yet or are attackable
+https://github.com/PSPReverse/amd-sev-migration-attack
+* KVM is unsupported when running as an SEV guest
+* PCI passthrough is not supported
+
+Example Configuration:
+
+----
+# qm set <vmid> -memory_encryption type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1
+----
+
+*SEV Parameters*
+
+*type* defines the encryption technology ("type=" is not necessary):
+currently-supported: *sev*
+and in the future: sev-snp, mktme
+
+*reduced-phys-bios*, *cbitpos* and *policy* correspond to the variables with the
+same name in qemu.
+
+*reduced-phys-bios* and *cbitpos* are system specific and can be read out
+with QMP. If not set, qm starts a dummy-vm to read QMP
+for these variables out and saves them to config.
+
+*policy* can be calculated with
+https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf[AMD SEV API Specification Chapter 3]
+
+To use 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))
+
+*Check if SEV is working on the Guest*
+
+Method 1 - dmesg:
+
+Output should look like this.
+
+----
+# dmesg | grep -i sev
+AMD Memory Encryption Features active: SEV
+----
+
+Method 2 - MSR 0xc0010131 (MSR_AMD64_SEV):
+
+Output should be 1.
+
+----
+# apt install msr-tools
+# modprobe msr
+# rdmsr -a 0xc0010131
+1
+----
+
+Links:
+
+* https://github.com/AMDESE/AMDSEV
+* https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
+* https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
+* https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html
+
+// Commented because cannot be tested without new EPYC-CPU
+// AMD SEV-SNP
+// ^^^^^^^^^^^
+// * SEV-SNP needs EPYC 7003 "Milan" processors.
+// * SEV-SNP should in Kernel 5.19:
+// https://www.phoronix.com/scan.php?page=news_item&px=AMD-SEV-SNP-Arrives-Linux-5.19
 
 [[qm_network_device]]
 Network Device
-- 
2.30.2

Re: [pve-devel] [PATCH docs v2 2/2] added Memory Encryption documentation

[Matthias Heiserer]

Inline, I have some suggestions regarding wording and spelling

On 11.11.2022 15:27, Markus Frank wrote:
> added AMD SEV documentation for "[PATCH qemu-server] QEMU AMD SEV
> enable"
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   qm.adoc | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 113 insertions(+)
> 
> diff --git a/qm.adoc b/qm.adoc
> index e7d0c07..5ba43a2 100644
> --- a/qm.adoc
> +++ b/qm.adoc
> @@ -598,6 +598,119 @@ systems.
>   When allocating RAM to your VMs, a good rule of thumb is always to leave 1GB
>   of RAM available to the host.
>   
> +[[qm_memory_encryption]]
> +Memory Encryption
> +~~~~~~~~~~~~~~~~~
> +
> +[[qm_memory_encryption_sev]]
> +AMD SEV
> +^^^^^^^
> +
> +Memory Encryption per VM using AES-128 Encryption and the AMD Secure Processor.
> +See https://developer.amd.com/sev/[AMD SEV]
> +
> +*Host-Requirements:*
> +
> +* AMD EPYC/Ryzen PRO CPU
> +* configured SEV BIOS settings on Host Machine
> +* add "kvm_amd.sev=1" to kernel parameters if not enabled by default
> +* add "mem_encrypt=on" to kernel parameters if you want encrypt memory on the
"to encrypt" or "encrypted"
> +host (SME)
> +see https://www.kernel.org/doc/Documentation/x86/amd-memory-encryption.txt
> +* maybe increase SWIOTLB see https://github.com/AMDESE/AMDSEV#faq-4
> +
> +To check if SEV is enabled on Host-Machine search for `sev` in dmesg
I think "on the host" would be the best wording
> +and print out the sev kernel parameter of kvm_amd:
> +
> +----
> +# dmesg | grep -i sev
> +[...] ccp 0000:45:00.1: sev enabled
> +[...] ccp 0000:45:00.1: SEV API: <buildversion>
> +[...] SEV supported: <number> ASIDs
> +[...] SEV-ES supported: <number> ASIDs
> +# cat /sys/module/kvm_amd/parameters/sev
> +Y
> +----
> +
> +*Guest-VM-Requirements:*
Also here, we spell guest/host without hyphen. Maybe just drop the VM, 
i.e. "Guest Requirements"
> +
> +* edk2-OVMF
> +* advisable to use Q35
> +* The guest operating system inside the VM must contain SEV-support
> +* if there are problems while booting (stops at blank/splash screen or "Guest has not
> +initialized the display (yet)") try to add virtio-rng and/or set "freeze: 1"
> +so that you wait a few seconds before you click on *Resume* to boot.
There's inconsistent capitalization
> +
> +*Limitations:*
> +
> +* Because the memory is encrypted the memory usage on host is always wrong
> +* Operations that involve saving or restoring memory like snapshots
> +& live migration do not work yet or are attackable
> +https://github.com/PSPReverse/amd-sev-migration-attack
> +* KVM is unsupported when running as an SEV guest
> +* PCI passthrough is not supported
> +
> +Example Configuration:
> +
> +----
> +# qm set <vmid> -memory_encryption type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1
> +----
> +
> +*SEV Parameters*
I'd use the same format as e.g. in "10.13.3. Options".
Regarding the following sentences: Consistently end them with a dot or 
without, don't mix.
> +
> +*type* defines the encryption technology ("type=" is not necessary):
> +currently-supported: *sev*
> +and in the future: sev-snp, mktme
> +
> +*reduced-phys-bios*, *cbitpos* and *policy* correspond to the variables with the
> +same name in qemu.
> +
> +*reduced-phys-bios* and *cbitpos* are system specific and can be read out
> +with QMP. If not set, qm starts a dummy-vm to read QMP
> +for these variables out and saves them to config.
> +
> +*policy* can be calculated with
> +https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf[AMD SEV API Specification Chapter 3]
> +
> +To use 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))
> +
> +*Check if SEV is working on the Guest*
> +
> +Method 1 - dmesg:
> +
> +Output should look like this.
> +
> +----
> +# dmesg | grep -i sev
> +AMD Memory Encryption Features active: SEV
> +----
> +
> +Method 2 - MSR 0xc0010131 (MSR_AMD64_SEV):
> +
> +Output should be 1.
> +
> +----
> +# apt install msr-tools
> +# modprobe msr
> +# rdmsr -a 0xc0010131
> +1
> +----
> +
> +Links:
> +
> +* https://github.com/AMDESE/AMDSEV
> +* https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
> +* https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
> +* https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html
In case you haven't done so, you could make a snapshot of these pages 
with archive.org or similarly, so there's a backup if they ever get removed.
> +
> +// Commented because cannot be tested without new EPYC-CPU
> +// AMD SEV-SNP
> +// ^^^^^^^^^^^
> +// * SEV-SNP needs EPYC 7003 "Milan" processors.
> +// * SEV-SNP should in Kernel 5.19:
> +// https://www.phoronix.com/scan.php?page=news_item&px=AMD-SEV-SNP-Arrives-Linux-5.19
>   
>   [[qm_network_device]]
>   Network Device

Re: [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@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);

Re: [pve-devel] [PATCH docs v2 2/2] added Memory Encryption documentation

[Fiona Ebner]

Am 11.11.22 um 15:27 schrieb Markus Frank:
> +* if there are problems while booting (stops at blank/splash screen or "Guest has not
> +initialized the display (yet)") try to add virtio-rng and/or set "freeze: 1"
> +so that you wait a few seconds before you click on *Resume* to boot.

Doesn't sound very nice from a user perspective. Do you have an idea
what's going on there? Can we automatically add some other kvm parameter
to make it work (better) when SEV is configured?

> +
> +*Limitations:*
> +
> +* Because the memory is encrypted the memory usage on host is always wrong
> +* Operations that involve saving or restoring memory like snapshots
> +& live migration do not work yet or are attackable
> +https://github.com/PSPReverse/amd-sev-migration-attack

What happens if a user attempts a migration or snapshot right now? I
think we should cleanly error out before even attempting if it can't
work (yet).

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

[Markus Frank]

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);

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

[Fiona Ebner]

Am 17.11.22 um 11:50 schrieb Markus Frank:>>> @@ -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

You can also just remove the eval to make the error propagate ;) And I
guess the whole helper could be replaced at the call side with an inline
    parse_property_string($memory_encryption_fmt, $value) if $value
but no big deal.

>>> +    # 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

The run_command helper can take an "input => $input" parameter. Haven't
actually used it myself, but there are existing examples in our code base :)