From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id B88851FF13F for ; Thu, 29 Jan 2026 13:16:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0A30A7FF; Thu, 29 Jan 2026 13:17:18 +0100 (CET) Message-ID: <3f6ae5e9-7871-4220-b759-15b71720a4b6@proxmox.com> Date: Thu, 29 Jan 2026 13:16:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [pve-devel] [PATCH manager 07/10] ui: qemu: make machine panels/fields architecture aware To: Fiona Ebner , Proxmox VE development discussion References: <20260128123035.2576774-1-d.csapak@proxmox.com> <20260128123035.2576774-8-d.csapak@proxmox.com> <60129254-d70c-48d5-8f3c-10d3e320da28@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <60129254-d70c-48d5-8f3c-10d3e320da28@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1769688928401 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.031 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: RY7M67BHVPME3XWQ2QJKHRRCTSZJ4MUC X-Message-ID-Hash: RY7M67BHVPME3XWQ2QJKHRRCTSZJ4MUC X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 >> --- >> 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'; > } > } >