Re: [pve-devel] [PATCH qemu-server] fix #3784: Parameter for guest vIOMMU

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 25.02.22 13:00, Markus Frank wrote:
> added a Parameter for vIOMMU so nobody has to use a github-script
> or change the kvm command on every vm boot.
> The args parameter doesn't work because the -device parameter(kvm)
> with intel-iommu has to be placed before the other arguments.
> 

some short sentence what vIOMMU is used for could be nice to have for
context.

> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  PVE/QemuServer.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 42f0fbd..a09dd2e 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -647,6 +647,12 @@ EODESCR
>  	    ." remove disk operations.",
>  	default => 0,
>      },
> +    viommu => {
> +	optional => 1,
> +	type => 'boolean',
> +	description => "enable guest vIOMMU",
> +	default => 0,
> +    },

I'd like to avoid having too many separate config properties where sensible.
qemu-server obv. failed already a bit with that, but that's now its legacy ^^

Anyhow, do you think it could make sense to either:
* have this a format-string (`property: foo=1,bar=2,baz=42`) added to an existing
  option
* use a more general `machine-flags` that already starts out as format-string so
  that we can reuse this in the future, e.g., for memory encryption (qemu sets that
  also on its machine param) or the like?

>      bios => {
>  	optional => 1,
>  	type => 'string',
> @@ -3427,6 +3433,9 @@ sub config_to_command {
>      my $winversion = windows_version($ostype);
>      my $kvm = $conf->{kvm};
>      my $nodename = nodename();
> +    my $viommu = $conf->{viommu};
> +
> +    push @$devices, '-device', 'intel-iommu,intremap=on,caching-mode=on' if $viommu;
>  
>      my $arch = get_vm_arch($conf);
>      my $kvm_binary = get_command_for_arch($arch);
> @@ -4021,6 +4030,8 @@ sub config_to_command {
>      }
>      push @$machineFlags, "type=${machine_type_min}";
>  
> +    push @$machineFlags, 'accel=kvm,kernel-irqchip=split' if $viommu;

We set accel= already elsewhere, did you check for possible conflicts that we could
catch earlier (e.g., config set) and tell the user?

> +
>      push @$cmd, @$devices;
>      push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags);
>      push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags);