From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 413D01FF2C7 for ; Wed, 24 Jul 2024 15:05:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F2D0E1C38C; Wed, 24 Jul 2024 15:05:57 +0200 (CEST) Message-ID: Date: Wed, 24 Jul 2024 15:05:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Markus Frank References: <20240529122348.1267369-1-m.frank@proxmox.com> <20240529122348.1267369-3-m.frank@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20240529122348.1267369-3-m.frank@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [suse.com, cpuconfig.pm] Subject: Re: [pve-devel] [PATCH qemu-server v11 2/5] config: add AMD SEV support 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 29.05.24 um 14:23 schrieb Markus Frank: > Signed-off-by: Markus Frank Reviewed-by: Fiona Ebner with some (style) nits that can still be addressed: > diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm > index 33f7524..2542aa2 100644 > --- a/PVE/QemuServer/CPUConfig.pm > +++ b/PVE/QemuServer/CPUConfig.pm > @@ -3,9 +3,10 @@ package PVE::QemuServer::CPUConfig; > use strict; > use warnings; > > +use JSON; Style nit: please add a blank line between non-PVE and PVE modules > @@ -773,6 +806,54 @@ sub get_cpu_bitness { > die "unsupported architecture '$arch'\n"; > } > > +sub get_hw_capabilities { > + # Get reduced-phys-bits & cbitpos from host-hw-capabilities.json > + my $filename = '/run/qemu-server/host-hw-capabilities.json'; > + if (! -e $filename) { > + run_command("/usr/libexec/qemu-server/query-machine-capabilities"); > + } > + my $json_text = PVE::Tools::file_get_contents($filename); > + ($json_text) = $json_text =~ /(.*)/; # untaint json text > + my $hw_capabilities = decode_json($json_text); Nit: could use eval and add context to errors from decode_json() > + return $hw_capabilities; > +} > + > +sub get_amd_sev_object { > + my ($conf) = @_; Nit: could also pass the three properties from the config it actually needs instead of passing the whole config for a more explicit function signature. > + > + if (!$conf->{bios} || ($conf->{bios} && $conf->{bios} ne 'ovmf')) { > + die "To use SEV, you need to change the BIOS to OVMF.\n"; > + } > + die "To use SEV, you need to add an efidisk.\n" if (!$conf->{efidisk0}); I'd move the checks to below the ones for the hardware capability. Otherwise a user might see the error about BIOS first, change it, try again and then fail with the error about lacking hardware support. I'd also use "AMD SEV" instead of just "SEV". What happens if there is no EFI disk (i.e. the temporary efivars disk is used)? Style nit: superfluous parentheses in post-if > + > + my $amd_sev_conf = PVE::JSONSchema::parse_property_string($sev_fmt, $conf->{amd_sev}); > + my $sev_hw_caps = get_hw_capabilities()->{'amd-sev'}; > + > + if (!$sev_hw_caps->{'sev-support'}) { > + die "Your CPU does not support AMD SEV!\n";> + } > + if ($amd_sev_conf->{type} eq 'es' && !$sev_hw_caps->{'sev-support-es'}) { > + die "Your CPU does not support AMD SEV-ES!\n"; > + } No need for the exclamation marks for those two error messages IMHO. > + > + my $sev_mem_object = 'sev-guest,id=sev0' > + .',cbitpos='.$sev_hw_caps->{cbitpos} > + .',reduced-phys-bits='.$sev_hw_caps->{'reduced-phys-bits'}; Style nit: usually this is written as my $a = "foo"; $a .= "bar"; in our code base. > + > + # guest policy bit calculation as described here: > + # https://documentation.suse.com/sles/15-SP5/html/SLES-amd-sev/article-amd-sev.html#table-guestpolicy > + my $policy = 0b0000; > + $policy += 0b0001 if ($amd_sev_conf->{'no-debug'}); > + $policy += 0b0010 if ($amd_sev_conf->{'no-key-sharing'}); > + $policy += 0b0100 if ($amd_sev_conf->{type} eq 'es'); Style nit: superfluous parentheses in post-if, also below > + # disable migration with bit 3 nosend to prevent amd-sev-migration-attack > + $policy += 0b1000; > + > + $sev_mem_object .= ',policy='.sprintf("%#x", $policy); > + $sev_mem_object .= ',kernel-hashes=on' if ($amd_sev_conf->{'kernel-hashes'}); > + return $sev_mem_object; > +} > + > __PACKAGE__->register(); > __PACKAGE__->init(); > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel