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

[Dominik Csapak <d.csapak@proxmox.com>]

On 1/29/26 12:11 PM, Fiona Ebner wrote:
> 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.

yep noticed it too now, the reason is that the arch and nodename are not 
set via 'binds' yet when the 'onMachineChange' is triggering
(at least not everytime, maybe a race condition? i swear it
worked at least once on my machine ;) )

I'll have to change how we get/save the arch/nodename there.
When that works, it'll not show any versions currently because
the backend does not return any 'virt' models.

i think we should fix that in the backend. I'd also add an 'arch'
parameter for the machines like you sent for the cputype


> 
>> 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.

make sense

> 
>> +
>> +        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.

as i wrote in the other mail, i think i'll change this completely
namely don't let each component calculate this seperately

> 
>> +
>> +    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.

i think this was just done as a heuristic without trying to replicate 
the backend logic too tightly, but i might be misremembering

> 
> 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/
i'll check it out

> 
>>                       }
>> -                    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';
>              }
>          }
>