Re: [pve-devel] [PATCH manager v9 3/3] ui: machine: add viommu ComboBox

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

On 11/04/2024 12:48, Markus Frank wrote:
> Added a proxmoxKVComboBox for selecting a vIOMMU implementation for a
> VM. If i440fx is selected, a hint tells that q35 is required for Intel vIOMMU.
> 
> The UI also needs to parse the new machine parameter as PropertyString.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  www/manager6/qemu/MachineEdit.js | 45 ++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
> index f928c80c..45f3d34d 100644
> --- a/www/manager6/qemu/MachineEdit.js
> +++ b/www/manager6/qemu/MachineEdit.js
> @@ -1,6 +1,7 @@
>  Ext.define('PVE.qemu.MachineInputPanel', {
>      extend: 'Proxmox.panel.InputPanel',
>      xtype: 'pveMachineInputPanel',
> +    onlineHelp: 'qm_system_settings',
nit: this could be done in a separate patch.

>  
>      controller: {
>  	xclass: 'Ext.app.ViewController',

> @@ -40,12 +44,30 @@ Ext.define('PVE.qemu.MachineInputPanel', {
>  	    delete values.delete;
>  	}
>  	delete values.version;
> +	if (values.machine === undefined) {
> +	    if (values.viommu) {
> +		delete values.delete;
> +		values.machine = "pc";
> +	    } else {
> +		values.delete = "machine";
> +	    }
> +	}
> +	if (values.viommu) {
> +	    values.machine += ",viommu=" + values.viommu;
> +	}
> +	if (values.delete === "viommu") {
> +	    delete values.delete;
> +	}
> +	delete values.viommu;

can't we use printPropertyString here (with a bit less code preparing the object
passed to it), or at least try making it a bit less convoluted..

>  	    fieldLabel: gettext('Note'),
>  	    value: gettext('Machine version change may affect hardware layout and settings in the guest OS.'),
>  	},
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    fieldLabel: gettext('vIOMMU'),
> +	    name: 'viommu',
> +	    reference: 'viommu',
> +	    value: '__default__',
> +	    comboItems: [
> +		['__default__', Proxmox.Utils.defaultText + ' (None)'],
> +		['intel', 'Intel'],
> +		['virtio', 'VirtIO'],
> +	    ],
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    name: 'q35Hint',
> +	    reference: 'q35Hint',
> +	    userCls: 'pmx-hint',
> +	    value: gettext('Intel vIOMMU needs the q35 machine type'),

maybe we could mark the field invalid (if that text is returned in the
validity check it will be shown as tooltip) instead of using the hint?

> +	    hidden: true,
> +	},
>      ],
>  });
>