public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
@ 2024-04-15  8:50 Markus Frank
  2024-04-15  8:50 ` [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings Markus Frank
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Markus Frank @ 2024-04-15  8:50 UTC (permalink / raw)
  To: pve-devel

Added a proxmoxKVComboBox for selecting a vIOMMU implementation for a VM.
If i440fx is selected, another ComboBox will be enabled/visible that does not
have the Intel option, as Intel-vIOMMU is not compatible with i440fx.

Uses the new machine property-string from the qemu-server's "config: define
machine schema as property-string" commit and the viommu option added in the
qemu-server's "fix #3784: config: Parameter for guest vIOMMU + test-cases"
commit.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 www/manager6/qemu/MachineEdit.js | 62 +++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
index f928c80c..48c72c1d 100644
--- a/www/manager6/qemu/MachineEdit.js
+++ b/www/manager6/qemu/MachineEdit.js
@@ -2,6 +2,15 @@ Ext.define('PVE.qemu.MachineInputPanel', {
     extend: 'Proxmox.panel.InputPanel',
     xtype: 'pveMachineInputPanel',
 
+    viewModel: {
+	data: {
+	    type: '__default__',
+	},
+	formulas: {
+	    q35: get => get('type') === 'q35',
+	},
+    },
+
     controller: {
 	xclass: 'Ext.app.ViewController',
 	control: {
@@ -35,17 +44,29 @@ Ext.define('PVE.qemu.MachineInputPanel', {
     },
 
     onGetValues: function(values) {
+	if (values.delete === 'machine' && values.viommu) {
+	    delete values.delete;
+	    values.machine = 'pc';
+	}
 	if (values.version && values.version !== 'latest') {
 	    values.machine = values.version;
 	    delete values.delete;
 	}
 	delete values.version;
-	return values;
+	if (values.delete === 'machine' && !values.viommu) {
+	    return values;
+	}
+	let ret = {};
+	ret.machine = PVE.Parser.printPropertyString(values, 'machine');
+	return ret;
     },
 
     setValues: function(values) {
 	let me = this;
 
+	let machineConf = PVE.Parser.parsePropertyString(values.machine, 'type');
+	values.machine = machineConf.type;
+
 	me.isWindows = values.isWindows;
 	if (values.machine === 'pc') {
 	    values.machine = '__default__';
@@ -58,6 +79,9 @@ Ext.define('PVE.qemu.MachineInputPanel', {
 		values.version = 'pc-q35-5.1';
 	    }
 	}
+
+	values.viommu = machineConf.viommu || '__default__';
+
 	if (values.machine !== '__default__' && values.machine !== 'q35') {
 	    values.version = values.machine;
 	    values.machine = values.version.match(/q35/) ? 'q35' : '__default__';
@@ -78,6 +102,9 @@ Ext.define('PVE.qemu.MachineInputPanel', {
 	    ['__default__', PVE.Utils.render_qemu_machine('')],
 	    ['q35', 'q35'],
 	],
+	bind: {
+	    value: '{type}',
+	},
     },
 
     advancedItems: [
@@ -113,6 +140,39 @@ Ext.define('PVE.qemu.MachineInputPanel', {
 	    fieldLabel: gettext('Note'),
 	    value: gettext('Machine version change may affect hardware layout and settings in the guest OS.'),
 	},
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'viommu',
+	    fieldLabel: gettext('vIOMMU'),
+	    reference: 'viommu-q35',
+	    deleteEmpty: false,
+	    value: '__default__',
+	    comboItems: [
+		['__default__', Proxmox.Utils.defaultText + ' (None)'],
+		['intel', 'Intel'],
+		['virtio', 'VirtIO'],
+	    ],
+	    bind: {
+		hidden: '{!q35}',
+		disabled: '{!q35}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'viommu',
+	    fieldLabel: gettext('vIOMMU'),
+	    reference: 'viommu-i440fx',
+	    deleteEmpty: false,
+	    value: '__default__',
+	    comboItems: [
+		['__default__', Proxmox.Utils.defaultText + ' (None)'],
+		['virtio', 'VirtIO'],
+	    ],
+	    bind: {
+		hidden: '{q35}',
+		disabled: '{q35}',
+	    },
+	},
     ],
 });
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-15  8:50 [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Markus Frank
@ 2024-04-15  8:50 ` Markus Frank
  2024-04-22 13:13   ` Fiona Ebner
  2024-04-22 13:11 ` [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Fiona Ebner
  2024-04-22 13:11 ` [pve-devel] applied: " Dominik Csapak
  2 siblings, 1 reply; 12+ messages in thread
From: Markus Frank @ 2024-04-15  8:50 UTC (permalink / raw)
  To: pve-devel

---
 www/manager6/qemu/MachineEdit.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
index 48c72c1d..ee2b2dac 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',
 
     viewModel: {
 	data: {
-- 
2.39.2





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
  2024-04-15  8:50 [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Markus Frank
  2024-04-15  8:50 ` [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings Markus Frank
@ 2024-04-22 13:11 ` Fiona Ebner
  2024-04-22 13:16   ` Dominik Csapak
  2024-04-22 13:11 ` [pve-devel] applied: " Dominik Csapak
  2 siblings, 1 reply; 12+ messages in thread
From: Fiona Ebner @ 2024-04-22 13:11 UTC (permalink / raw)
  To: Proxmox VE development discussion, Markus Frank

Am 15.04.24 um 10:50 schrieb Markus Frank:
> Added a proxmoxKVComboBox for selecting a vIOMMU implementation for a VM.
> If i440fx is selected, another ComboBox will be enabled/visible that does not
> have the Intel option, as Intel-vIOMMU is not compatible with i440fx.
> 

Just wondering, is it possible to bind the comboItems instead of having
two different comboboxes?

Should we display some hint that Intel can/should also be used even if
you have an AMD? Maybe even just in the text we display, like "Intel
(also used for AMD)" but hope somebody can come up with something better.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] applied: [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
  2024-04-15  8:50 [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Markus Frank
  2024-04-15  8:50 ` [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings Markus Frank
  2024-04-22 13:11 ` [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Fiona Ebner
@ 2024-04-22 13:11 ` Dominik Csapak
  2 siblings, 0 replies; 12+ messages in thread
From: Dominik Csapak @ 2024-04-22 13:11 UTC (permalink / raw)
  To: Proxmox VE development discussion, Markus Frank

applied both patches, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-15  8:50 ` [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings Markus Frank
@ 2024-04-22 13:13   ` Fiona Ebner
  2024-04-22 13:18     ` Dominik Csapak
  0 siblings, 1 reply; 12+ messages in thread
From: Fiona Ebner @ 2024-04-22 13:13 UTC (permalink / raw)
  To: Proxmox VE development discussion, Markus Frank

Am 15.04.24 um 10:50 schrieb Markus Frank:
> ---
>  www/manager6/qemu/MachineEdit.js | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
> index 48c72c1d..ee2b2dac 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',
>  

Why not the more accurate 'qm_machine_type' (was introduced in pve-docs
>= 8.1.0)?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
  2024-04-22 13:11 ` [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Fiona Ebner
@ 2024-04-22 13:16   ` Dominik Csapak
  2024-04-22 13:24     ` Fiona Ebner
  0 siblings, 1 reply; 12+ messages in thread
From: Dominik Csapak @ 2024-04-22 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner, Markus Frank

On 4/22/24 15:11, Fiona Ebner wrote:
> Am 15.04.24 um 10:50 schrieb Markus Frank:
>> Added a proxmoxKVComboBox for selecting a vIOMMU implementation for a VM.
>> If i440fx is selected, another ComboBox will be enabled/visible that does not
>> have the Intel option, as Intel-vIOMMU is not compatible with i440fx.
>>
> 
just applied both patches here, so i'm replying

> Just wondering, is it possible to bind the comboItems instead of having
> two different comboboxes?

no thats currently not possible, since the comboitems become a store with records
changing the comboItems after initialization does not do anything

(we could change that of course, but this is good enough for now imo)

> 
> Should we display some hint that Intel can/should also be used even if
> you have an AMD? Maybe even just in the text we display, like "Intel
> (also used for AMD)" but hope somebody can come up with something better.
> 
> 

mhh.. i mean it is a virtual device, so should we also add this info
for e.g. e1000 devices?

since it's in the advanced section and it is documented in pve-docs,
i'd leave it out here (we can still add a notice later if users
are confused, but most users won't use/need it anyway)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-22 13:13   ` Fiona Ebner
@ 2024-04-22 13:18     ` Dominik Csapak
  2024-04-22 13:19       ` Fiona Ebner
  2024-04-22 13:21       ` Thomas Lamprecht
  0 siblings, 2 replies; 12+ messages in thread
From: Dominik Csapak @ 2024-04-22 13:18 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner, Markus Frank

On 4/22/24 15:13, Fiona Ebner wrote:
> Am 15.04.24 um 10:50 schrieb Markus Frank:
>> ---
>>   www/manager6/qemu/MachineEdit.js | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
>> index 48c72c1d..ee2b2dac 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',
>>   
> 
> Why not the more accurate 'qm_machine_type' (was introduced in pve-docs
>> = 8.1.0)?
> 
> 

you're right, would be even better (did not realize that existed), i/you can send/push a follow up?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-22 13:18     ` Dominik Csapak
@ 2024-04-22 13:19       ` Fiona Ebner
  2024-04-22 13:21       ` Thomas Lamprecht
  1 sibling, 0 replies; 12+ messages in thread
From: Fiona Ebner @ 2024-04-22 13:19 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion, Markus Frank

Am 22.04.24 um 15:18 schrieb Dominik Csapak:
> On 4/22/24 15:13, Fiona Ebner wrote:
>> Am 15.04.24 um 10:50 schrieb Markus Frank:
>>> ---
>>>   www/manager6/qemu/MachineEdit.js | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/www/manager6/qemu/MachineEdit.js
>>> b/www/manager6/qemu/MachineEdit.js
>>> index 48c72c1d..ee2b2dac 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',
>>>   
>>
>> Why not the more accurate 'qm_machine_type' (was introduced in pve-docs
>>> = 8.1.0)?
>>
>>
> 
> you're right, would be even better (did not realize that existed), i/you
> can send/push a follow up?
> 

Feel free to push a follow-up, I need to go in 5 minutes ;)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-22 13:18     ` Dominik Csapak
  2024-04-22 13:19       ` Fiona Ebner
@ 2024-04-22 13:21       ` Thomas Lamprecht
  2024-04-22 13:22         ` Dominik Csapak
  1 sibling, 1 reply; 12+ messages in thread
From: Thomas Lamprecht @ 2024-04-22 13:21 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak, Fiona Ebner,
	Markus Frank

Am 22/04/2024 um 15:18 schrieb Dominik Csapak:
> On 4/22/24 15:13, Fiona Ebner wrote:
>> Why not the more accurate 'qm_machine_type' (was introduced in pve-docs = 8.1.0)?
> 
> you're right, would be even better (did not realize that existed), i/you can send/push a follow up?
> 

both of you can push to this repo, for this little change I think doing so directly
without patch is fine


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings
  2024-04-22 13:21       ` Thomas Lamprecht
@ 2024-04-22 13:22         ` Dominik Csapak
  0 siblings, 0 replies; 12+ messages in thread
From: Dominik Csapak @ 2024-04-22 13:22 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Fiona Ebner,
	Markus Frank

On 4/22/24 15:21, Thomas Lamprecht wrote:
> Am 22/04/2024 um 15:18 schrieb Dominik Csapak:
>> On 4/22/24 15:13, Fiona Ebner wrote:
>>> Why not the more accurate 'qm_machine_type' (was introduced in pve-docs = 8.1.0)?
>>
>> you're right, would be even better (did not realize that existed), i/you can send/push a follow up?
>>
> 
> both of you can push to this repo, for this little change I think doing so directly
> without patch is fine

pushed a follow up


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
  2024-04-22 13:16   ` Dominik Csapak
@ 2024-04-22 13:24     ` Fiona Ebner
  2024-04-22 17:58       ` Thomas Lamprecht
  0 siblings, 1 reply; 12+ messages in thread
From: Fiona Ebner @ 2024-04-22 13:24 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion, Markus Frank

Am 22.04.24 um 15:16 schrieb Dominik Csapak:
> On 4/22/24 15:11, Fiona Ebner wrote:
> 
>>
>> Should we display some hint that Intel can/should also be used even if
>> you have an AMD? Maybe even just in the text we display, like "Intel
>> (also used for AMD)" but hope somebody can come up with something better.
>>
>>
> 
> mhh.. i mean it is a virtual device, so should we also add this info
> for e.g. e1000 devices?
> 

But with Intel/AMD, one is inclined to assume there is a dichotomy.
Personally, I'd go in assuming the "Intel" setting is wrong with my AMD
CPU and so might many users.

> since it's in the advanced section and it is documented in pve-docs,
> i'd leave it out here (we can still add a notice later if users
> are confused, but most users won't use/need it anyway)

Yes, we could also wait. But if the confusion can be avoided/reduced
without much effort, I think it's worth doing up-front.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox
  2024-04-22 13:24     ` Fiona Ebner
@ 2024-04-22 17:58       ` Thomas Lamprecht
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Lamprecht @ 2024-04-22 17:58 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner, Dominik Csapak,
	Markus Frank

Am 22/04/2024 um 15:24 schrieb Fiona Ebner:
> Am 22.04.24 um 15:16 schrieb Dominik Csapak:
>> On 4/22/24 15:11, Fiona Ebner wrote:
>>> Should we display some hint that Intel can/should also be used even if
>>> you have an AMD? Maybe even just in the text we display, like "Intel
>>> (also used for AMD)" but hope somebody can come up with something better.
>>
>> mhh.. i mean it is a virtual device, so should we also add this info
>> for e.g. e1000 devices?

Intel e1000 is a network card where no real relation to similar models from
AMD exist, nor is the network market a duopoly like the (x86_64) CPU market;
so this comparison does not really work IMO.

> Personally, I'd go in assuming the "Intel" setting is wrong with my AMD
> CPU and so might many users.
> 
>> since it's in the advanced section and it is documented in pve-docs,
>> i'd leave it out here (we can still add a notice later if users
>> are confused, but most users won't use/need it anyway)
> 
> Yes, we could also wait. But if the confusion can be avoided/reduced
> without much effort, I think it's worth doing up-front.

Yeah, definitively agreed, this is guaranteed to be a source of confusion
otherwise. I changed the display value to "Intel (AMD Compatible)".
Albeit now I'm thinking that it might have been slightly better to
lowercase "compatible", oh well..

https://git.proxmox.com/?p=pve-manager.git;a=commit;h=216398458d4be8781155f7d64835a38971258793


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-04-22 17:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-15  8:50 [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Markus Frank
2024-04-15  8:50 ` [pve-devel] [PATCH manager v10 2/2] ui: machine: add link to documentation of the system settings Markus Frank
2024-04-22 13:13   ` Fiona Ebner
2024-04-22 13:18     ` Dominik Csapak
2024-04-22 13:19       ` Fiona Ebner
2024-04-22 13:21       ` Thomas Lamprecht
2024-04-22 13:22         ` Dominik Csapak
2024-04-22 13:11 ` [pve-devel] [PATCH manager v10 1/2] ui: machine: add viommu ComboBox Fiona Ebner
2024-04-22 13:16   ` Dominik Csapak
2024-04-22 13:24     ` Fiona Ebner
2024-04-22 17:58       ` Thomas Lamprecht
2024-04-22 13:11 ` [pve-devel] applied: " Dominik Csapak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal