Re: [pve-devel] [PATCH manager 07/10] ui: qemu: make machine panels/fields architecture aware

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

Am 28.01.26 um 1:30 PM schrieb Dominik Csapak:
> For this, introduce a new 'QemuMachineSelector', which does the same
> thing as the scsihw selector by filtering the kv store with a predefined
> list for each architecture.
> 
> Since the backend default of the machine is actually different for
> x86_64 vs aarch64, there is some logic to handle the 'default' value
> for the machine (iow. replace 'pc' with the default machine for each
> architecture)
> 

Setting a machine version for an existing aarch64 VM is broken, e.g. the
result will be "pc-i440fx-10.1" instead.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/Makefile                    |  1 +
>  www/manager6/Utils.js                    | 18 ++++++-
>  www/manager6/form/QemuMachineSelector.js | 64 ++++++++++++++++++++++++
>  www/manager6/qemu/HardwareView.js        |  3 +-
>  www/manager6/qemu/MachineEdit.js         | 63 +++++++++++++++++------
>  www/manager6/qemu/SystemEdit.js          | 10 ++--
>  6 files changed, 136 insertions(+), 23 deletions(-)
>  create mode 100644 www/manager6/form/QemuMachineSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 4558d53e..7670b95d 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -62,6 +62,7 @@ JSSRC= 							\
>  	form/PreallocationSelector.js			\
>  	form/PrivilegesSelector.js			\
>  	form/QemuBiosSelector.js			\
> +	form/QemuMachineSelector.js			\
>  	form/SDNControllerSelector.js			\
>  	form/SDNZoneSelector.js				\
>  	form/SDNVnetSelector.js				\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index de1ee0ba..613a7113 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -509,8 +509,22 @@ Ext.define('PVE.Utils', {
>              return agentstring;
>          },
>  
> -        render_qemu_machine: function (value) {
> -            return value || Proxmox.Utils.defaultText + ' (i440fx)';
> +        defaultMachines: {
> +            x86_64: 'pc',
> +            aarch64: 'virt',
> +        },
> +
> +        machineText: {
> +            pc: 'i440fx',
> +        },

These new entries should mention QEMU somewhere in their name, because
it's in the very general/stuffed Utils class.

> +
> +        render_qemu_machine: function (value, arch = 'x86_64') {
> +            if (!value) {
> +                let machine = PVE.Utils.defaultMachines[arch];
> +                let machineText = PVE.Utils.machineText[machine] ?? machine;
> +                return `${Proxmox.Utils.defaultText} (${machineText})`;
> +            }
> +            return value;
>          },
>  
>          render_qemu_bios: function (value) {
> diff --git a/www/manager6/form/QemuMachineSelector.js b/www/manager6/form/QemuMachineSelector.js
> new file mode 100644
> index 00000000..2852b585
> --- /dev/null
> +++ b/www/manager6/form/QemuMachineSelector.js
> @@ -0,0 +1,64 @@
> +Ext.define('PVE.form.QemuMachineSelector', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: 'widget.pveQemuMachineSelector',
> +
> +    comboItems: [
> +        ['__default__', PVE.Utils.render_qemu_machine('')],
> +        ['q35', 'q35'],
> +    ],
> +
> +    machinesPerArchitecture: {
> +        x86_64: ['__default__', 'q35'], // __default__ is i440fx
> +        aarch64: ['__default__'], // __default__ is virt
> +    },
> +
> +    // defaults to hostArch
> +    arch: undefined,
> +
> +    // depends on the node
> +    hostArch: 'x86_64',

Sounds like it should start as undefined and we should require a
nodename to be present at init time then and set hostArch accordingly?
Otherwise, it seems a bit brittle and might be wrong if setNodename() is
forgotten to be called by a use site.

> +
> +    nodename: undefined,
> +
> +    setNodename: function (nodename) {
> +        let me = this;
> +        let node = PVE.data.ResourceStore.getNodeById(nodename);
> +        if (node) {
> +            me.hostArch = node.data.architecture;
> +            me.setArch(me.arch); // recalculate the filter
> +        }
> +    },
> +
> +    setArch: function (arch) {
> +        let me = this;
> +        if (arch === '__default__') {
> +            arch = undefined;
> +        }
> +        me.arch = arch;
> +        arch ??= me.hostArch;
> +        let wasValid = me.isValid();
> +        me.store.clearFilter();
> +        let allowedMachines = me.machinesPerArchitecture[arch];
> +        me.store.addFilter((rec) => allowedMachines.indexOf(rec.data.key) !== -1);
> +        // update default value with new arch
> +        let record = me.store.findRecord('key', '__default__');
> +        if (record) {
> +            record.set('value', PVE.Utils.render_qemu_machine('', arch));
> +            record.commit();
> +        }
> +        let isValid = me.isValid();
> +
> +        // for some reason, adding/changing filters does not trigger this, even though
> +        // it show the field as invalid, so simply track and fire the event manually.
> +        if (wasValid !== isValid) {
> +            me.fireEvent('validitychange', isValid);
> +        }
> +    },
> +
> +    initComponent: function () {
> +        var me = this;
> +
> +        me.callParent();
> +        me.setArch(me.arch);
> +    },
> +});
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index b7cc7856..1e2cd026 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -202,13 +202,14 @@ Ext.define('PVE.qemu.HardwareView', {
>                  defaultValue: '',
>                  renderer: function (value, metaData, record, rowIndex, colIndex, store, pending) {
>                      let ostype = me.getObjectValue('ostype', undefined, pending);
> +                    let arch = PVE.Utils.getArchitecture(me.getObjectValue('arch'), nodename);
>                      if (
>                          PVE.Utils.is_windows(ostype) &&
>                          (!value || value === 'pc' || value === 'q35')
>                      ) {
>                          return value === 'q35' ? 'pc-q35-5.1' : 'pc-i440fx-5.1';

Unrelated to the patch, but I noticed now that this is actually wrong.
Since QEMU 9.1+ the backend defaults to the creation version if present.

But related to the series, it should default to virt for ARM of course,
which it doesn't yet, but I sent a patch:
https://lore.proxmox.com/pve-devel/20260129110607.65416-1-f.ebner@proxmox.com/T/

>                      }
> -                    return PVE.Utils.render_qemu_machine(value);
> +                    return PVE.Utils.render_qemu_machine(value, arch);
>                  },
>              },
>              scsihw: {
> diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
> index 1b1989e8..89d13886 100644
> --- a/www/manager6/qemu/MachineEdit.js
> +++ b/www/manager6/qemu/MachineEdit.js
> @@ -23,8 +23,16 @@ Ext.define('PVE.qemu.MachineInputPanel', {
>              let me = this;
>              let version = me.lookup('version');
>              let store = version.getStore();
> +
>              let oldRec = store.findRecord('id', version.getValue(), 0, false, false, true);
> -            let type = value === 'q35' ? 'q35' : 'i440fx';
> +
> +            let vm = me.getViewModel();
> +            let arch = PVE.Utils.getArchitecture(vm.get('arch'), vm.get('nodename'));
> +            let defaultMachine = PVE.Utils.defaultMachines[arch];
> +            if (defaultMachine === 'pc') {
> +                defaultMachine = 'i440fx'; // the default in the backend is 'pc' which means 'i440fx' for the qemu machinetype'

Style nit: please move the comment to its own line. Comment has a
trailing excessive single quote

> +            }
> +            let type = value === 'q35' ? 'q35' : defaultMachine;
>              store.clearFilter();
>              store.addFilter((val) => val.data.id === 'latest' || val.data.type === type);
>              if (!me.getView().isWindows) {
> @@ -45,9 +53,12 @@ Ext.define('PVE.qemu.MachineInputPanel', {
>      },
>  
>      onGetValues: function (values) {
> +        let me = this;
> +        let arch = PVE.Utils.getArchitecture(values.arch, me.nodename);
> +        delete values.arch;
>          if (values.delete === 'machine' && values.viommu) {
>              delete values.delete;
> -            values.machine = 'pc';
> +            values.machine = PVE.Utils.defaultMachines[arch];
>          }
>          if (values.version && values.version !== 'latest') {
>              values.machine = values.version;
> @@ -68,8 +79,10 @@ Ext.define('PVE.qemu.MachineInputPanel', {
>          let machineConf = PVE.Parser.parsePropertyString(values.machine, 'type');
>          values.machine = machineConf.type;
>  
> +        let arch = PVE.Utils.getArchitecture(values.arch, me.nodename);
> +        let defaultMachine = PVE.Utils.defaultMachines[arch];
>          me.isWindows = values.isWindows;
> -        if (values.machine === 'pc') {
> +        if (values.machine === defaultMachine) {
>              values.machine = '__default__';
>          }
>  

Similar to above, unrelated but the pinning does not match the backend,
but will also need to be adapted once the default for virt is fixed in
the backend.

        if (me.isWindows) {
            if (values.machine === '__default__') {
                values.version = 'pc-i440fx-5.1';
            } else if (values.machine === 'q35') {
                values.version = 'pc-q35-5.1';
            }
        }