Re: [pve-devel] [PATCH qemu-server v11 2/5] config: add AMD SEV support

[Fiona Ebner <f.ebner@proxmox.com>]

Am 29.05.24 um 14:23 schrieb Markus Frank:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

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