Re: [pve-devel] [PATCH qemu-server v6 1/4] feature #3784: Parameter for guest vIOMMU & machine as property-string

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

Am 22.08.23 um 14:40 schrieb Markus Frank:
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index bf1de17..013792d 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -118,12 +118,32 @@ PVE::JSONSchema::register_standard_option('pve-qm-stateuri', {
>      optional => 1,
>  });
>  
> -PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
> +my $machine_fmt = {
> +    type => {
> +	default_key => 1,
>  	description => "Specifies the QEMU machine type.",
>  	type => 'string',
>  	pattern => '(pc|pc(-i440fx)?-\d+(\.\d+)+(\+pve\d+)?(\.pxe)?|q35|pc-q35-\d+(\.\d+)+(\+pve\d+)?(\.pxe)?|virt(?:-\d+(\.\d+)+)?(\+pve\d+)?)',
>  	maxLength => 40,
> +	format_description => 'machine type',
>  	optional => 1,
> +    },
> +    viommu => {
> +	type => 'boolean',
> +	description => "Enable/disable guest vIOMMU"
> +	    ." (needs kvm to be enabled and q35 to be set as machine type).",
> +	default => 0,
> +	optional => 1,
> +    },
> +};
> +
> +PVE::JSONSchema::register_format('pve-qemu-machine-fmt', $machine_fmt);
> +
> +PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
> +    description => "Specify the QEMU machine type & enable/disable vIOMMU.",
> +    type => 'string',
> +    optional => 1,
> +    format => PVE::JSONSchema::get_format('pve-qemu-machine-fmt'),
>  });
>  
>  # FIXME: remove in favor of just using the INotify one, it's cached there exactly the same way
> @@ -2133,6 +2153,31 @@ sub parse_watchdog {
>      return $res;
>  }
>  
> +sub parse_machine {
> +    my ($value) = @_;
> +
> +    return if !$value;
> +
> +    my $res = parse_property_string($machine_fmt, $value);
> +    return $res;
> +}
> +
> +sub check_machine_config {
> +    my ($conf, $machine_conf) = @_;
> +    my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
> +    my $kvm = $conf->{kvm};
> +    my $arch = get_vm_arch($conf);
> +    $kvm //= 1 if is_native($arch);
> +    if ($machine_conf->{viommu} && (!$kvm || !$q35)) {
> +	die "to use vIOMMU please enable kvm and set the machine type to q35\n";
> +    }
> +}
> +
> +sub print_machine {
> +    my ($machine_conf) = @_;
> +    return PVE::JSONSchema::print_property_string($machine_conf, $machine_fmt);
> +}
> +

Was there any reason why you didn't put the above, i.e. format
definition/parsing/printing in the QemuServer::Machine module instead
like I suggested in my review of v5?

> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index d9429ed..bfbde59 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -15,7 +15,8 @@ our $PVE_MACHINE_VERSION = {
>  sub machine_type_is_q35 {
>      my ($conf) = @_;
>  
> -    return $conf->{machine} && ($conf->{machine} =~ m/q35/) ? 1 : 0;
> +    my $machine_conf = PVE::QemuServer::parse_machine($conf->{machine});
> +    return $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
>  }
>  
>  sub current_from_query_machines {
> @@ -120,7 +121,8 @@ sub qemu_machine_pxe {
>  
>      my $machine =  get_current_qemu_machine($vmid);
>  
> -    if ($conf->{machine} && $conf->{machine} =~ m/\.pxe$/) {
> +    my $machine_conf = PVE::QemuServer::parse_machine($conf->{machine});
> +    if ($machine_conf->{type} && $machine_conf->{type} =~ m/\.pxe$/) {
>  	$machine .= '.pxe';
>      }
>  

Because there still are these cyclic calls back into the QemuServer
module (and you would need the cylcic use statement which is what we
want to avoid).