Am 29.01.25 um 16:53 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.scha...@proxmox.com>
> ---
> PVE/QemuServer.pm | 83 +++---------------------
> PVE/QemuServer/Makefile | 1 +
> PVE/QemuServer/RNG.pm | 135 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 145 insertions(+), 74 deletions(-)
> create mode 100644 PVE/QemuServer/RNG.pm
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5cde94a1..93e65825 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -61,6 +61,7 @@ use PVE::QemuServer::Memory qw(get_current_memory);
> use PVE::QemuServer::Monitor qw(mon_cmd);
> use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr
> print_pcie_root_port parse_hostpci);
> use PVE::QemuServer::QMPHelpers qw(qemu_deviceadd qemu_devicedel
> qemu_objectadd qemu_objectdel);
> +use PVE::QemuServer::RNG;
> use PVE::QemuServer::USB;
>
> my $have_sdn;
> @@ -249,39 +250,6 @@ my $spice_enhancements_fmt = {
> },
> };
>
> -my $rng_fmt = {
> - source => {
> - type => 'string',
> - enum => ['/dev/urandom', '/dev/random', '/dev/hwrng'],
> - default_key => 1,
> - description => "The file on the host to gather entropy from. In most
> cases '/dev/urandom'"
> - ." should be preferred over '/dev/random' to avoid
> entropy-starvation issues on the"
> - ." host. Using urandom does *not* decrease security in any
> meaningful way, as it's"
> - ." still seeded from real entropy, and the bytes provided will most
> likely be mixed"
> - ." with real entropy on the guest as well. '/dev/hwrng' can be used
> to pass through"
> - ." a hardware RNG from the host.",
> - },
> - max_bytes => {
> - type => 'integer',
> - description => "Maximum bytes of entropy allowed to get injected into
> the guest every"
> - ." 'period' milliseconds. Prefer a lower value when using
> '/dev/random' as source. Use"
> - ." `0` to disable limiting (potentially dangerous!).",
> - optional => 1,
> -
> - # default is 1 KiB/s, provides enough entropy to the guest to avoid
> boot-starvation issues
> - # (e.g. systemd etc...) while allowing no chance of overwhelming the
> host, provided we're
> - # reading from /dev/urandom
> - default => 1024,
> - },
> - period => {
> - type => 'integer',
> - description => "Every 'period' milliseconds the entropy-injection quota
> is reset, allowing"
> - ." the guest to retrieve another 'max_bytes' of entropy.",
> - optional => 1,
> - default => 1000,
> - },
> -};
> -
> my $meta_info_fmt = {
> 'ctime' => {
> type => 'integer',
Does not apply, because this got moved recently ;)
> @@ -724,7 +692,7 @@ EODESCR
> },
> rng0 => {
> type => 'string',
> - format => $rng_fmt,
> + format => $PVE::QemuServer::RNG::rng_fmt,
> description => "Configure a VirtIO-based Random Number Generator.",
> optional => 1,
> },
Could instead use the standard option you define in the RNG module.
> @@ -2078,16 +2046,6 @@ sub parse_vga {
> return $res;
> }
>
> -sub parse_rng {
> - my ($value) = @_;
> -
> - return if !$value;
> -
> - my $res = eval { parse_property_string($rng_fmt, $value) };
> - warn $@ if $@;
> - return $res;
> -}
> -
> sub parse_meta_info {
> my ($value) = @_;
>
Does not apply, because this got moved recently ;)
> @@ -3940,20 +3898,14 @@ sub config_to_command {
> }
> }
>
> - my $rng = $conf->{rng0} ? parse_rng($conf->{rng0}) : undef;
> + my $rng = $conf->{rng0} ? PVE::QemuServer::RNG::parse_rng($conf->{rng0})
> : undef;
> if ($rng && $version_guard->(4, 1, 2)) {
> - check_rng_source($rng->{source});
> -
> - my $max_bytes = $rng->{max_bytes} // $rng_fmt->{max_bytes}->{default};
> - my $period = $rng->{period} // $rng_fmt->{period}->{default};
> - my $limiter_str = "";
> - if ($max_bytes) {
> - $limiter_str = ",max-bytes=$max_bytes,period=$period";
> - }
> -
> - my $rng_addr = print_pci_addr("rng0", $bridges, $arch, $machine_type);
> - push @$devices, '-object', "rng-random,filename=$rng->{source},id=rng0";
> - push @$devices, '-device',
> "virtio-rng-pci,rng=rng0$limiter_str$rng_addr";
> + my $rng_object = PVE::QemuServer::RNG::print_rng_object('rng0', $rng);
> + my $rng_device = PVE::QemuServer::RNG::print_rng_device(
> + 'rng0', $rng, $bridges, $arch, $machine_type
> + );
Style nit: that's not how multiline function calls are usually wrapped
in our Perl code base:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Arguments
> + push @$devices, '-object', $rng_object;
> + push @$devices, '-device', $rng_device;
> }
>
> my $spice_port;
Would be nice to have the moving to a dedicated module be separate from
adding the new helpers.
> @@ -4235,23 +4187,6 @@ sub config_to_command {
> return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cmd;
> }
>
> -sub check_rng_source {
> - my ($source) = @_;
> -
> - # mostly relevant for /dev/hwrng, but doesn't hurt to check others too
> - die "cannot create VirtIO RNG device: source file '$source' doesn't
> exist\n"
> - if ! -e $source;
> -
> - my $rng_current = '/sys/devices/virtual/misc/hw_random/rng_current';
> - if ($source eq '/dev/hwrng' && file_read_firstline($rng_current) eq
> 'none') {
> - # Needs to abort, otherwise QEMU crashes on first rng access. Note that
> rng_current cannot
> - # be changed to 'none' manually, so once the VM is past this point,
> it's no longer an issue.
> - die "Cannot start VM with passed-through RNG device: '/dev/hwrng'
> exists, but"
> - ." '$rng_current' is set to 'none'. Ensure that a compatible
> hardware-RNG is attached"
> - ." to the host.\n";
> - }
> -}
> -
> sub spice_port {
> my ($vmid) = @_;
>
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 89d12091..72c287fc 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -1,4 +1,5 @@
> SOURCES=PCI.pm \
> + RNG.pm \
> USB.pm \
> Memory.pm \
> ImportDisk.pm \
> diff --git a/PVE/QemuServer/RNG.pm b/PVE/QemuServer/RNG.pm
> new file mode 100644
> index 00000000..f7a62f3b
> --- /dev/null
> +++ b/PVE/QemuServer/RNG.pm
> @@ -0,0 +1,135 @@
> +package PVE::QemuServer::RNG;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::QemuServer::PCI qw(print_pci_addr);
> +use PVE::JSONSchema;
> +use PVE::Tools qw(file_read_firstline);
> +use base 'Exporter';
Style nit: The QemuServer::PCI module should be grouped below and I'd
prefer having a blank before it:
https://lore.proxmox.com/pve-devel/e24881cf-bfd2-4063-bde2-99f41031f...@proxmox.com/
Having the 'use base' be last is fine, but I'd also prefer a blank
before it.
> +
> +our @EXPORT_OK = qw(
> +parse_rng
> +check_rng_source
> +print_rng_device
> +print_rng_object
> +);
> +
> +our $rng_fmt = {
> + source => {
> + type => 'string',
> + enum => ['/dev/urandom', '/dev/random', '/dev/hwrng'],
> + default_key => 1,
> + optional => 1,
Adding the optional should not be part of this patch.
> + description => "The file on the host to gather entropy from. In most
> cases '/dev/urandom'"
> + ." should be preferred over '/dev/random' to avoid
> entropy-starvation issues on the"
> + ." host. Using urandom does *not* decrease security in any
> meaningful way, as it's"
> + ." still seeded from real entropy, and the bytes provided will most
> likely be mixed"
> + ." with real entropy on the guest as well. '/dev/hwrng' can be used
> to pass through"
> + ." a hardware RNG from the host.",
The part about /dev/random is outdated as you pointed out in v1 ;)
> + },
> + max_bytes => {
> + type => 'integer',
> + description => "Maximum bytes of entropy allowed to get injected into
> the guest every"
> + ." 'period' milliseconds. Prefer a lower value when using
> '/dev/random' as source. Use"
> + ." `0` to disable limiting (potentially dangerous!).",
The part about /dev/random is outdated as you pointed out in v1 ;)
> + optional => 1,
> +
> + # default is 1 KiB/s, provides enough entropy to the guest to avoid
> boot-starvation issues
> + # (e.g. systemd etc...) while allowing no chance of overwhelming the
> host, provided we're
> + # reading from /dev/urandom
> + default => 1024,
> + },
> + period => {
> + type => 'integer',
> + description => "Every 'period' milliseconds the entropy-injection quota
> is reset, allowing"
> + ." the guest to retrieve another 'max_bytes' of entropy.",
> + optional => 1,
> + default => 1000,
> + },
> +};
> +
> +PVE::JSONSchema::register_format('pve-qm-rng', $rng_fmt);
Since you register the format here, you don't need to make $rng_fmt
shared with 'our' or? (Still need to include the module in QemuServer.pm
to have registering come first).
> +
> +our $rngdesc = {
> + type => 'string',
> + format => $rng_fmt,
> + optional => 1,
> + description => "Configure a VirtIO-based Random Number Generator.",
> +};
> +PVE::JSONSchema::register_standard_option('pve-qm-rng', $rngdesc);
> +
> +sub parse_rng {
> + my ($value) = @_;
> +
> + return if !$value;
> +
> + my $res = eval { PVE::JSONSchema::parse_property_string($rng_fmt,
> $value) };
> + warn $@ if $@;
> +
> + my $source = $res->{source};
Unused variable, should be part of a later patch.
> +
> + return $res;
> +}
> +
> +sub check_rng_source {
> + my ($source) = @_;
> +
> + # mostly relevant for /dev/hwrng, but doesn't hurt to check others too
> + die "cannot create VirtIO RNG device: source file '$source' doesn't
> exist\n"
> + if ! -e $source;
> +
> + my $rng_current = '/sys/devices/virtual/misc/hw_random/rng_current';
> + if ($source eq '/dev/hwrng' && file_read_firstline($rng_current) eq
> 'none') {
> + # Needs to abort, otherwise QEMU crashes on first rng access. Note that
> rng_current cannot
> + # be changed to 'none' manually, so once the VM is past this point,
> it's no longer an issue.
> + die "Cannot start VM with passed-through RNG device: '/dev/hwrng'
> exists, but"
> + ." '$rng_current' is set to 'none'. Ensure that a compatible
> hardware-RNG is attached"
> + ." to the host.\n";
> + }
> +}
> +
> +sub get_rng_source_path {
> + my ($rng) = @_;
> +
> + my $source = $rng->{source};
> +
> + if (defined($source)) {
> + return $source;
> + }
> +
> + return;
> +}
> +
> +sub print_rng_device {
I'd add _commandline to reduce potential for confusion
> + my ($id, $rng, $bridges, $arch, $machine) = @_;
> +
> + return if !$rng;
Isn't failing better? IMHO caller should first check that it has
something to print rather than check the result.
> +
> + my $source_path = get_rng_source_path($rng);
> + check_rng_source($source_path);
Since the source is not part of the result, maybe not check it here, but
only in print_rng_object()?
> +
> + my $max_bytes = $rng->{max_bytes} // $rng_fmt->{max_bytes}->{default};
> + my $period = $rng->{period} // $rng_fmt->{period}->{default};
> + my $limiter_str = "";
> + if ($max_bytes) {
> + $limiter_str = ",max-bytes=$max_bytes,period=$period";
> + }
> +
> + my $rng_addr = print_pci_addr($id, $bridges, $arch, $machine);
> +
> + return "virtio-rng-pci,rng=$id$limiter_str$rng_addr";
> +}
> +
> +sub print_rng_object {
I'd add _commandline to reduce potential for confusion
> + my ($id, $rng) = @_;
> +
> + return if !$rng;
Isn't failing better? IMHO caller should first check that it has
something to print rather than check the result.
> +
> + my $source_path = get_rng_source_path($rng);
> + check_rng_source($source_path);
> +
> + return "rng-random,filename=$source_path,id=$id";
> +}
> +
> +1;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
