[PATCH manager/qemu-server 0/8] Add API and UI for custom CPU models

[PATCH manager/qemu-server 0/8] Add API and UI for custom CPU models

[Arthur Bied-Charreton]

This is picked up from an old series [0] by Stefan Reiter.

As of before this series, the only way to create custom CPU models is by
editing `/etc/pve/virtual-guest/cpu-models.conf` manually.

This can be cumbersome for a few reasons, e.g., due to the fact that flags
misconfigurations are only caught when starting the VM.

`cpu-flags` endpoint:
The `cpu-flags` endpoint previously returned a list of hardcoded flags,
which is both non-exhaustive (some flags I should be able to set are missing),
and partly incorrect (some flags my host(s) do not support set are returned).
This is limiting and can lead to misconfigurations. The updated endpoint
intersects all flags QEMU accepts as `-cpu` arguments with all flags the host
hardware/emulation actually supports. This way, if I am able to set a flag in
the UI, I can be confident that the VM will actually be able to start.

Custom CPU model CRUD functionality:
Expose CRUD endpoints and UI flow to interact with `cpu-models.conf`. For each
flag, show a list of the cluster nodes supporting it, and only expose flags that
at least one node supports to avoid misconfigurations. Filter flags by
acceleration type (KVM/TCG).

[0] https://lore.proxmox.com/pve-devel/20211028114150.3245864-1-s.reiter@proxmox.com/


pve-manager:

Arthur Bied-Charreton (5):
  ui: VMCPUFlagSelector: Fix unknownFlags behaviour
  ui: CPUModelSelector: Fix dirty state on default
  ui: CPUModelSelector: Allow filtering out custom models
  ui: Add basic custom CPU model editor
  ui: Add CPU flag editor for custom models

 www/manager6/Makefile                  |   3 +
 www/manager6/dc/CPUTypeEdit.js         |  99 ++++++++++++++++
 www/manager6/dc/CPUTypeView.js         | 139 ++++++++++++++++++++++
 www/manager6/dc/Config.js              |   6 +
 www/manager6/form/CPUModelSelector.js  |  18 ++-
 www/manager6/form/PhysBitsSelector.js  | 153 +++++++++++++++++++++++++
 www/manager6/form/VMCPUFlagSelector.js | 135 ++++++++++++++++++----
 7 files changed, 529 insertions(+), 24 deletions(-)
 create mode 100644 www/manager6/dc/CPUTypeEdit.js
 create mode 100644 www/manager6/dc/CPUTypeView.js
 create mode 100644 www/manager6/form/PhysBitsSelector.js


qemu-server:

Arthur Bied-Charreton (3):
  qemu: Add helpers for new custom models endpoints
  api: qemu: Extend cpu-flags endpoint to return actually supported
    flags
  api: qemu: Add CRUD handlers for custom CPU models

 src/PVE/API2/Qemu/CPU.pm        | 236 +++++++++++++++++++++++++++++++-
 src/PVE/API2/Qemu/CPUFlags.pm   | 108 ++++++++++++++-
 src/PVE/QemuServer/CPUConfig.pm |  35 ++++-
 3 files changed, 374 insertions(+), 5 deletions(-)


Summary over all repositories:
  10 files changed, 903 insertions(+), 29 deletions(-)

-- 
Generated by murpp 0.9.0

[PATCH pve-manager 1/8] ui: VMCPUFlagSelector: Fix unknownFlags behaviour

[Arthur Bied-Charreton]

...and typo in name.

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-6-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/form/VMCPUFlagSelector.js | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/www/manager6/form/VMCPUFlagSelector.js b/www/manager6/form/VMCPUFlagSelector.js
index 922fcfa6..74b1a2c4 100644
--- a/www/manager6/form/VMCPUFlagSelector.js
+++ b/www/manager6/form/VMCPUFlagSelector.js
@@ -14,7 +14,7 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
     scrollable: 'y',
     height: 200,
 
-    unkownFlags: [],
+    unknownFlags: [],
 
     store: {
         type: 'store',
@@ -62,32 +62,32 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
             }
         });
 
-        flags += me.unkownFlags.join(';');
+        flags += (me.unknownFlags.length > 0 ? ';' : '') + me.unknownFlags.join(';');
 
         return flags;
     },
 
-    // Adjusts the store for the current value and determines the unkown flags based on what the
+    // Adjusts the store for the current value and determines the unknown flags based on what the
     // store does not know.
     adjustStoreForValue: function () {
         let me = this;
         let store = me.getStore();
         let value = me.value;
 
-        me.unkownFlags = [];
+        me.unknownFlags = [];
 
         store.getData().each((rec) => rec.set('state', '='));
 
         let flags = value ? value.split(';') : [];
         flags.forEach(function (flag) {
             let sign = flag.substr(0, 1);
-            flag = flag.substr(1);
+            let flagName = flag.substr(1);
 
-            let rec = store.findRecord('name', flag, 0, false, true, true);
+            let rec = store.findRecord('name', flagName, 0, false, true, true);
             if (rec !== null) {
                 rec.set('state', sign);
             } else {
-                me.unkownFlags.push(flag);
+                me.unknownFlags.push(flag);
             }
         });
 
-- 
2.47.3

[PATCH pve-manager 2/8] ui: CPUModelSelector: Fix dirty state on default

[Arthur Bied-Charreton]

'originalValue' is set to null in case it's the default, but getValue
returns an empty string. This means that when editing a VM's CPU config
when the model is 'default', the form would always be marked dirty.

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-7-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/form/CPUModelSelector.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/www/manager6/form/CPUModelSelector.js b/www/manager6/form/CPUModelSelector.js
index 2ebd08eb..737b6ff4 100644
--- a/www/manager6/form/CPUModelSelector.js
+++ b/www/manager6/form/CPUModelSelector.js
@@ -39,7 +39,11 @@ Ext.define('PVE.form.CPUModelSelector', {
         ],
         width: 360,
     },
-
+    getValue: function () {
+        let me = this;
+        let val = me.callParent();
+        return val === '' ? null : val;
+    },
     store: {
         autoLoad: true,
         model: 'PVE.data.CPUModel',
-- 
2.47.3

[PATCH pve-manager 3/8] ui: CPUModelSelector: Allow filtering out custom models

[Arthur Bied-Charreton]

Add 'allowCustom' configuration parameter. When set to false, only
default CPU models will be shown.

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-8-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/form/CPUModelSelector.js | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/www/manager6/form/CPUModelSelector.js b/www/manager6/form/CPUModelSelector.js
index 737b6ff4..2154ff46 100644
--- a/www/manager6/form/CPUModelSelector.js
+++ b/www/manager6/form/CPUModelSelector.js
@@ -19,7 +19,9 @@ Ext.define('PVE.form.CPUModelSelector', {
     autoSelect: false,
 
     deleteEmpty: true,
-
+    config: {
+        allowCustom: true,
+    },
     listConfig: {
         columns: [
             {
@@ -100,4 +102,11 @@ Ext.define('PVE.form.CPUModelSelector', {
             },
         },
     },
+    initComponent: function () {
+        let me = this;
+        me.callParent();
+        if (!me.allowCustom) {
+            me.getStore().addFilter({ filterFn: (rec) => !rec.data.custom });
+        }
+    },
 });
-- 
2.47.3

[PATCH pve-manager 4/8] ui: Add basic custom CPU model editor

[Arthur Bied-Charreton]

Add components for creating/updating custom CPU models, allowing to set
the values that would normally have to be set manually in
`/etc/pve/virtual-guest/cpu-models.conf` [0].

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-9-s.reiter@proxmox.com/

[0] https://pve.proxmox.com/wiki/Manual:_cpu-models.conf

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/Makefile                 |   3 +
 www/manager6/dc/CPUTypeEdit.js        |  90 +++++++++++++++
 www/manager6/dc/CPUTypeView.js        | 139 +++++++++++++++++++++++
 www/manager6/dc/Config.js             |   6 +
 www/manager6/form/PhysBitsSelector.js | 153 ++++++++++++++++++++++++++
 5 files changed, 391 insertions(+)
 create mode 100644 www/manager6/dc/CPUTypeEdit.js
 create mode 100644 www/manager6/dc/CPUTypeView.js
 create mode 100644 www/manager6/form/PhysBitsSelector.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 4558d53e..fbde2327 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -58,6 +58,7 @@ JSSRC= 							\
 	form/PCISelector.js				\
 	form/PCIMapSelector.js				\
 	form/PermPathSelector.js			\
+	form/PhysBitsSelector.js			\
 	form/PoolSelector.js				\
 	form/PreallocationSelector.js			\
 	form/PrivilegesSelector.js			\
@@ -170,6 +171,8 @@ JSSRC= 							\
 	dc/CmdMenu.js					\
 	dc/Config.js					\
 	dc/CorosyncLinkEdit.js				\
+	dc/CPUTypeEdit.js				\
+	dc/CPUTypeView.js				\
 	dc/GroupEdit.js					\
 	dc/GroupView.js					\
 	dc/Guests.js					\
diff --git a/www/manager6/dc/CPUTypeEdit.js b/www/manager6/dc/CPUTypeEdit.js
new file mode 100644
index 00000000..8cf508b4
--- /dev/null
+++ b/www/manager6/dc/CPUTypeEdit.js
@@ -0,0 +1,90 @@
+Ext.define('PVE.dc.CPUTypeEdit', {
+    extend: 'Proxmox.window.Edit',
+    alias: ['widget.pveCpuTypeEdit'],
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('CPU Type'),
+
+    // Avoid default-focusing the reported model dropdown while still
+    // focusing the name textfield if it is editable
+    defaultFocus: 'textfield',
+
+    height: 600,
+    width: 800,
+
+    cbindData: {
+        cputype: '',
+        isCreate: (cfg) => !cfg.cputype,
+    },
+
+    cbind: {
+        autoLoad: (get) => !get('isCreate'),
+        url: (get) => `/api2/extjs/nodes/localhost/capabilities/qemu/cpu/model/${get('cputype')}`,
+        method: (get) => (get('isCreate') ? 'POST' : 'PUT'),
+        isCreate: (get) => get('isCreate'),
+    },
+
+    getValues: function () {
+        let me = this;
+        let values = me.callParent();
+
+        PVE.Utils.delete_if_default(values, 'reported-model', '', me.isCreate);
+        PVE.Utils.delete_if_default(values, 'hv-vendor-id', '', me.isCreate);
+        PVE.Utils.delete_if_default(values, 'phys-bits', '', me.isCreate);
+        PVE.Utils.delete_if_default(values, 'flags', '', me.isCreate);
+
+        if (me.isCreate && !values.cputype.match(/^custom-/)) {
+            values.cputype = 'custom-' + values.cputype;
+        }
+
+        return values;
+    },
+
+    items: [
+        {
+            xtype: 'inputpanel',
+            column1: [
+                {
+                    xtype: 'pmxDisplayEditField',
+                    fieldLabel: gettext('Name'),
+                    cbind: {
+                        editable: '{isCreate}',
+                        value: '{cputype}',
+                    },
+                    name: 'cputype',
+                    renderer: (val) => val.replace(/^custom-/, ''),
+                    allowBlank: false,
+                },
+                {
+                    xtype: 'CPUModelSelector',
+                    fieldLabel: gettext('Reported Model'),
+                    allowCustom: false,
+                    name: 'reported-model',
+                },
+                {
+                    xtype: 'textfield',
+                    fieldLabel: gettext('Hyper-V Vendor'),
+                    name: 'hv-vendor-id',
+                    allowBlank: true,
+                    emptyText: gettext('None'),
+                    maxLength: 12,
+                },
+            ],
+            column2: [
+                {
+                    xtype: 'checkbox',
+                    fieldLabel: gettext('Hidden'),
+                    name: 'hidden',
+                    inputValue: 1,
+                    uncheckedValue: 0,
+                },
+                {
+                    xtype: 'PhysBitsSelector',
+                    fieldLabel: gettext('Phys-Bits'),
+                    name: 'phys-bits',
+                },
+            ],
+
+        },
+    ],
+});
diff --git a/www/manager6/dc/CPUTypeView.js b/www/manager6/dc/CPUTypeView.js
new file mode 100644
index 00000000..c79ce690
--- /dev/null
+++ b/www/manager6/dc/CPUTypeView.js
@@ -0,0 +1,139 @@
+Ext.define('pve-custom-cpu-type', {
+    extend: 'Ext.data.Model',
+    fields: [
+        'cputype',
+        'reported-model',
+        'hv-vendor-id',
+        'flags',
+        'phys-bits',
+        { name: 'hidden', type: 'boolean' },
+    ],
+});
+
+Ext.define('PVE.dc.CPUTypeView', {
+    extend: 'Ext.grid.GridPanel',
+    alias: ['widget.pveCPUTypeView'],
+
+    onlineHelp: 'qm_cpu',
+
+    store: {
+        model: 'pve-custom-cpu-type',
+        proxy: {
+            type: 'proxmox',
+            url: '/api2/json/nodes/localhost/capabilities/qemu/cpu/model',
+        },
+        autoLoad: true,
+        sorters: ['cputype'],
+    },
+
+    controller: {
+        xclass: 'Ext.app.ViewController',
+
+        getSelection: function () {
+            let me = this;
+            let grid = me.getView();
+            let selection = grid.getSelection();
+            if (selection.length === 1) {
+                return selection[0].data;
+            }
+            return null;
+        },
+
+        showEditor: function (cputype) {
+            let me = this;
+            let param = cputype ? { cputype } : {};
+            let win = Ext.create('PVE.dc.CPUTypeEdit', param);
+            win.on('destroy', () => me.reload());
+            win.show();
+        },
+
+        onAdd: function () {
+            let me = this;
+            me.showEditor();
+        },
+
+        onEdit: function () {
+            let me = this;
+            let selection = me.getSelection();
+            me.showEditor(selection.cputype);
+        },
+
+        reload: function () {
+            let me = this;
+            me.getView().getStore().reload();
+        },
+    },
+
+    columns: [
+        {
+            header: gettext('Name'),
+            flex: 1,
+            dataIndex: 'cputype',
+            renderer: (val) => val.replace(/^custom-/, ''),
+        },
+        {
+            header: gettext('Reported Model'),
+            flex: 1,
+            dataIndex: 'reported-model',
+        },
+        {
+            header: gettext('Phys-Bits'),
+            flex: 1,
+            dataIndex: 'phys-bits',
+        },
+        {
+            header: gettext('Hidden'),
+            flex: 1,
+            dataIndex: 'hidden',
+        },
+        {
+            header: gettext('HyperV-Vendor'),
+            flex: 1,
+            dataIndex: 'hv-vendor-id',
+        },
+        {
+            header: gettext('Flags'),
+            flex: 2,
+            dataIndex: 'flags',
+        },
+    ],
+
+    tbar: [
+        {
+            text: gettext('Add'),
+            handler: 'onAdd',
+        },
+        '-',
+        {
+            xtype: 'proxmoxStdRemoveButton',
+            baseurl: '/api2/extjs/nodes/localhost/capabilities/qemu/cpu/model/',
+            getRecordName: (rec) => rec.data.cputype,
+            getUrl: function (rec) {
+                let me = this;
+                return me.baseurl + rec.data.cputype;
+            },
+            callback: 'reload',
+        },
+        {
+            text: gettext('Edit'),
+            handler: 'onEdit',
+        },
+    ],
+
+    selModel: {
+        xtype: 'rowmodel',
+    },
+
+    listeners: {
+        itemdblclick: function (_, rec) {
+            let me = this;
+            me.getController().showEditor(rec.data.cputype);
+        },
+    },
+
+    initComponent: function () {
+        let me = this;
+        me.callParent();
+        Proxmox.Utils.monStoreErrors(me, me.store);
+    },
+});
diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
index b5e27a21..629e4fc8 100644
--- a/www/manager6/dc/Config.js
+++ b/www/manager6/dc/Config.js
@@ -146,6 +146,12 @@ Ext.define('PVE.dc.Config', {
                     title: gettext('Replication'),
                     itemId: 'replication',
                 },
+                {
+                    xtype: 'pveCPUTypeView',
+                    iconCls: 'fa fa-microchip',
+                    title: gettext('Custom CPU models'),
+                    itemId: 'cputypes',
+                },
                 {
                     xtype: 'pveACLView',
                     title: gettext('Permissions'),
diff --git a/www/manager6/form/PhysBitsSelector.js b/www/manager6/form/PhysBitsSelector.js
new file mode 100644
index 00000000..78f932a6
--- /dev/null
+++ b/www/manager6/form/PhysBitsSelector.js
@@ -0,0 +1,153 @@
+Ext.define('PVE.form.PhysBitsSelector', {
+    extend: 'Ext.form.FieldContainer',
+    alias: 'widget.PhysBitsSelector',
+    mixins: ['Ext.form.field.Field'],
+
+    layout: 'vbox',
+    originalValue: '',
+
+    controller: {
+        xclass: 'Ext.app.ViewController',
+
+        onRadioChange: function (radio, value) {
+            let me = this;
+            if (value === undefined) {
+                return;
+            }
+
+            ['modeDefault', 'modeHost', 'modeCustom'].forEach(function (ref) {
+                let r = me.lookupReference(ref);
+                if (r !== radio) {
+                    r.suspendEvents();
+                    r.setValue(false);
+                    r.resumeEvents();
+                }
+            });
+
+            me.updateNumberField();
+        },
+
+        updateNumberField: function () {
+            let me = this;
+            let modeCustom = me.lookupReference('modeCustom');
+            let customNum = me.lookupReference('customNum');
+
+            customNum.setDisabled(!modeCustom.getValue());
+            me.getView().validate();
+        },
+
+        listen: {
+            component: {
+                '*': {
+                    change: function () {
+                        let me = this;
+                        me.getView().checkChange();
+                    },
+                },
+            },
+        },
+    },
+
+    getValue: function () {
+        let me = this;
+        let ctrl = me.getController();
+        if (ctrl.lookupReference('modeDefault').getValue()) {
+            return '';
+        } else if (ctrl.lookupReference('modeHost').getValue()) {
+            return 'host';
+        } else if (ctrl.lookupReference('modeCustom').getValue()) {
+            return ctrl.lookupReference('customNum').getValue();
+        }
+        return ''; // shouldn't happen
+    },
+
+    setValue: function (value) {
+        let me = this;
+        let ctrl = me.getController();
+        let modeField;
+
+        if (!value) {
+            modeField = ctrl.lookupReference('modeDefault');
+        } else if (value === 'host') {
+            modeField = ctrl.lookupReference('modeHost');
+        } else {
+            let customNum = ctrl.lookupReference('customNum');
+            customNum.setValue(value);
+            modeField = ctrl.lookupReference('modeCustom');
+        }
+
+        modeField.setValue(true);
+        me.checkChange();
+
+        return value;
+    },
+
+    getErrors: function () {
+        let me = this;
+        let ctrl = me.getController();
+        if (ctrl.lookupReference('modeCustom').getValue()) {
+            return ctrl.lookupReference('customNum').getErrors();
+        }
+        return [];
+    },
+
+    isValid: function () {
+        let me = this;
+        let ctrl = me.getController();
+        if (ctrl.lookupReference('modeCustom').getValue()) {
+            return ctrl.lookupReference('customNum').isValid();
+        }
+        return true;
+    },
+
+    items: [
+        {
+            xtype: 'radiofield',
+            boxLabel: gettext('Default'),
+            inputValue: 'default',
+            checked: true,
+            reference: 'modeDefault',
+            listeners: {
+                change: 'onRadioChange',
+            },
+            isFormField: false,
+        },
+        {
+            xtype: 'radiofield',
+            boxLabel: gettext('Host'),
+            inputValue: 'host',
+            reference: 'modeHost',
+            listeners: {
+                change: 'onRadioChange',
+            },
+            isFormField: false,
+        },
+        {
+            xtype: 'fieldcontainer',
+            layout: 'hbox',
+            items: [
+                {
+                    xtype: 'radiofield',
+                    boxLabel: gettext('Custom'),
+                    inputValue: 'custom',
+                    listeners: {
+                        change: 'onRadioChange',
+                    },
+                    reference: 'modeCustom',
+                    isFormField: false,
+                },
+                {
+                    xtype: 'numberfield',
+                    width: 60,
+                    margin: '0 0 0 10px',
+                    minValue: 8,
+                    maxValue: 64,
+                    reference: 'customNum',
+                    allowBlank: false,
+                    isFormField: false,
+                    disabled: true,
+                },
+            ],
+        },
+    ],
+});
-- 
2.47.3

[PATCH pve-manager 5/8] ui: Add CPU flag editor for custom models

[Arthur Bied-Charreton]

Add CPU flag editor to the CPUTypeEdit component, using the VMCPUFlagSelector
also used in the VM creation flow. By default, only show the CPU flags that
are currently meant to be shown in the VM creation window, see [0]. When in
CPUTypeEdit, show all available flags.

For each flag in VMCPUFlagSelector, also display which node(s) it is available
on to limit misconfigurations.

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-10-s.reiter@proxmox.com

[0] https://git.proxmox.com/?p=qemu-server.git;a=blob;f=src/PVE/QemuServer/CPUConfig.pm;h=32ec495422791422f20caa928d6b632b3de4fcd9;hb=refs/heads/master#l349

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/dc/CPUTypeEdit.js         |  11 ++-
 www/manager6/form/CPUModelSelector.js  |   1 +
 www/manager6/form/VMCPUFlagSelector.js | 121 ++++++++++++++++++++++---
 3 files changed, 117 insertions(+), 16 deletions(-)

diff --git a/www/manager6/dc/CPUTypeEdit.js b/www/manager6/dc/CPUTypeEdit.js
index 8cf508b4..d6e06601 100644
--- a/www/manager6/dc/CPUTypeEdit.js
+++ b/www/manager6/dc/CPUTypeEdit.js
@@ -84,7 +84,16 @@ Ext.define('PVE.dc.CPUTypeEdit', {
                     name: 'phys-bits',
                 },
             ],
-
+            columnB: [
+                {
+                    xtype: 'vmcpuflagselector',
+                    fieldLabel: gettext('Extra CPU flags'),
+                    name: 'flags',
+                    restrictToVMFlags: false,
+                    height: 380,
+                    hideHeaders: false,
+                },
+            ],
         },
     ],
 });
diff --git a/www/manager6/form/CPUModelSelector.js b/www/manager6/form/CPUModelSelector.js
index 2154ff46..683fa469 100644
--- a/www/manager6/form/CPUModelSelector.js
+++ b/www/manager6/form/CPUModelSelector.js
@@ -17,6 +17,7 @@ Ext.define('PVE.form.CPUModelSelector', {
     anyMatch: true,
     forceSelection: true,
     autoSelect: false,
+    triggerAction: 'query',
 
     deleteEmpty: true,
     config: {
diff --git a/www/manager6/form/VMCPUFlagSelector.js b/www/manager6/form/VMCPUFlagSelector.js
index 74b1a2c4..06c9d9f1 100644
--- a/www/manager6/form/VMCPUFlagSelector.js
+++ b/www/manager6/form/VMCPUFlagSelector.js
@@ -1,3 +1,19 @@
+const VM_CPU_FLAGS_SUBSET = {
+    aes: true,
+    'amd-no-ssb': true,
+    'amd-ssbd': true,
+    'hv-evmcs': true,
+    'hv-tlbflush': true,
+    ibpb: true,
+    'md-clear': true,
+    'nested-virt': true,
+    pcid: true,
+    pdpe1gb: true,
+    'spec-ctrl': true,
+    ssbd: true,
+    'virt-ssbd': true,
+};
+
 Ext.define('PVE.form.VMCPUFlagSelector', {
     extend: 'Ext.grid.Panel',
     alias: 'widget.vmcpuflagselector',
@@ -6,6 +22,10 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
         field: 'Ext.form.field.Field',
     },
 
+    config: {
+        restrictToVMFlags: true,
+    },
+
     disableSelection: true,
     columnLines: false,
     selectable: false,
@@ -17,27 +37,18 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
     unknownFlags: [],
 
     store: {
-        type: 'store',
-        fields: ['name', { name: 'state', defaultValue: '=' }, 'description'],
-        autoLoad: true,
+        fields: ['name', { name: 'state', defaultValue: '=' }, 'description', 'supported-on'],
+        autoLoad: false,
         proxy: {
             type: 'proxmox',
             url: '/api2/json/nodes/localhost/capabilities/qemu/cpu-flags',
+            extraParams: { accel: 'kvm' },
         },
         listeners: {
             update: function () {
                 this.commitChanges();
             },
-            refresh: function (store, eOpts) {
-                let me = this;
-                let view = me.view;
-
-                if (store.adjustedForValue !== view.value) {
-                    view.adjustStoreForValue();
-                }
-            },
         },
-        adjustedForValue: undefined,
     },
 
     getValue: function () {
@@ -86,14 +97,18 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
             let rec = store.findRecord('name', flagName, 0, false, true, true);
             if (rec !== null) {
                 rec.set('state', sign);
+                rec.commit();
             } else {
                 me.unknownFlags.push(flag);
             }
         });
 
-        store.adjustedForValue = value;
+        me.getView().refresh();
+    },
+    isDirty: function () {
+        let me = this;
+        return me.originalValue !== me.getValue();
     },
-
     setValue: function (value) {
         let me = this;
 
@@ -109,6 +124,7 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
     },
     columns: [
         {
+            text: gettext('State'),
             dataIndex: 'state',
             renderer: function (v) {
                 switch (v) {
@@ -125,6 +141,7 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
             width: 65,
         },
         {
+            text: gettext('Set'),
             xtype: 'widgetcolumn',
             dataIndex: 'state',
             width: 95,
@@ -171,22 +188,96 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
             },
         },
         {
+            text: gettext('Flag'),
             dataIndex: 'name',
             width: 100,
         },
         {
+            text: gettext('Description'),
             dataIndex: 'description',
+            sortable: false,
+            cellWrap: true,
+            flex: 3,
+        },
+        {
+            text: gettext('Supported On'),
+            dataIndex: 'supported-on',
             cellWrap: true,
             flex: 1,
+            renderer: (v) => (Array.isArray(v) ? v.join(', ') : ''),
         },
     ],
 
     initComponent: function () {
         let me = this;
 
+        me.unknownFlags = [];
         me.value = me.originalValue = '';
-        me.store.view = me;
+
+        me.dockedItems = [
+            {
+                xtype: 'toolbar',
+                dock: 'top',
+                items: [
+                    {
+                        xtype: 'tbtext',
+                        text: gettext('Acceleration:'),
+                        autoEl: {
+                            tag: 'span',
+                            'data-qtip': gettext(
+                                'A custom CPU model using acceleration-specific flags should only be assigned to VMs configured with the matching acceleration type, i.e., `kvm: off` for TCG, or `kvm: on` for KVM.',
+                            ),
+                        },
+                    },
+                    {
+                        xtype: 'radiogroup',
+                        layout: 'hbox',
+                        validateOnChange: false,
+                        items: [
+                            {
+                                boxLabel: 'KVM',
+                                inputValue: 'kvm',
+                                name: 'accel',
+                                checked: true,
+                                isFormField: false,
+                            },
+                            {
+                                boxLabel: 'TCG',
+                                inputValue: 'tcg',
+                                name: 'accel',
+                                margin: '0 0 0 10',
+                                isFormField: false,
+                            },
+                        ],
+                        listeners: {
+                            change: function (_, value) {
+                                let view = this.up('grid');
+                                let proxy = view.getStore().getProxy();
+                                let accel = value.accel;
+                                if (accel) {
+                                    proxy.setExtraParam('accel', accel);
+                                } else {
+                                    delete proxy.extraParams.accel;
+                                }
+                                view.getStore().load();
+                            },
+                        },
+                    },
+                ],
+            },
+        ];
 
         me.callParent(arguments);
+
+        me.getStore().on('load', function (store, _, success) {
+            if (success) {
+                if (me.restrictToVMFlags) {
+                    store.filterBy((rec) => VM_CPU_FLAGS_SUBSET[rec.get('name')] === true);
+                }
+                me.adjustStoreForValue();
+                me.checkDirty();
+            }
+        });
+        me.getStore().load();
     },
 });
-- 
2.47.3

[PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints

[Arthur Bied-Charreton]

Add functionality to lock & write the custom CPU config and some other
helpers that will be needed in custom CPU models CRUD endpoints.

Original patch:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-5-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/QemuServer/CPUConfig.pm | 35 ++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
index 32ec4954..2b6665a7 100644
--- a/src/PVE/QemuServer/CPUConfig.pm
+++ b/src/PVE/QemuServer/CPUConfig.pm
@@ -6,10 +6,10 @@ use warnings;
 use JSON;
 
 use PVE::JSONSchema qw(json_bool);
-use PVE::Cluster qw(cfs_register_file cfs_read_file);
+use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file);
 use PVE::ProcFSTools;
 use PVE::RESTEnvironment qw(log_warn);
-use PVE::Tools qw(run_command);
+use PVE::Tools qw(run_command lock_file);
 
 use PVE::QemuServer::Helpers qw(min_version);
 
@@ -51,6 +51,19 @@ sub load_custom_model_conf {
     return cfs_read_file($default_filename);
 }
 
+sub write_custom_model_conf {
+    my ($conf) = @_;
+    cfs_write_file($default_filename, $conf);
+}
+
+sub lock_cpu_config {
+    my ($code) = @_;
+    cfs_lock_file($default_filename, undef, $code);
+    if (my $err = $@) {
+        die $err;
+    }
+}
+
 #builtin models : reported-model is mandatory
 my $builtin_models_by_arch = {
     x86_64 => {
@@ -298,6 +311,19 @@ sub get_supported_cpu_flags {
     return $supported_cpu_flags_by_arch->{$arch};
 }
 
+sub description_by_flag {
+    my ($arch) = @_;
+    return { map { $_->{name} => $_->{description} } get_supported_cpu_flags($arch)->@* };
+}
+
+sub get_cpu_vendor {
+    my ($cputype, $arch) = @_;
+    $arch = $host_arch if !defined($arch);
+    my $retval = $cpu_models_by_arch->{$arch}->{$cputype}
+        or die "Built-in CPU type '$cputype' does not exist";
+    return $retval;
+}
+
 my $all_supported_cpu_flags = {};
 for my $arch ($supported_cpu_flags_by_arch->%*) {
     for my $flag ($supported_cpu_flags_by_arch->{$arch}->@*) {
@@ -375,6 +401,7 @@ my $cpu_fmt = {
         optional => 1,
     },
 };
+PVE::JSONSchema::register_standard_option('pve-qemu-cpu', $cpu_fmt);
 
 my $sev_fmt = {
     type => {
@@ -564,6 +591,7 @@ sub write_config {
 
         # saved in section header
         delete $model_conf->{cputype};
+        $model_conf->{type} = $class->type();
     }
 
     $class->SUPER::write_config($filename, $cfg);
@@ -612,7 +640,8 @@ sub get_cpu_models {
 
     my $conf = load_custom_model_conf();
     for my $custom_model (keys %{ $conf->{ids} }) {
-        my $reported_model = $conf->{ids}->{$custom_model}->{'reported-model'};
+        my $model = $conf->{ids}->{$custom_model};
+        my $reported_model = $model->{'reported-model'};
         $reported_model //= $cpu_fmt->{'reported-model'}->{default};
         my $vendor = $all_cpu_models->{$reported_model};
         push @$models,
-- 
2.47.3

[PATCH qemu-server 7/8] api: qemu: Extend cpu-flags endpoint to return actually supported flags

[Arthur Bied-Charreton]

Previously the endpoint returned a hardcoded list of flags. It now
returns flags that are both recognized by QEMU, and supported by at
least one cluster node to give a somewhat accurate picture of what flags
can actually be used on the current cluster.

The 'nested-virt` entry is prepended. An optional 'accel' parameter
filters results by virtualization type (kvm or tcg) to help avoid
misconfigurations when assigning flags to VMs with a specific
acceleration setting.

This deviates from the original patch [0] by adding the functionality to
the `cpu-flags` endpoint instead of adding new endpoints. This is
because we never need the understood/supported flags alone in the
frontend, only their intersection. This also improves the VM CPU flag
selector by letting users select from all possible flags in their
cluster.

When passed `aarch64` as argument for `arch`, the index returns an empty
list, which is consistent with the behavior from before this patch.

[0]
https://lore.proxmox.com/pve-devel/20211028114150.3245864-3-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Qemu/CPUFlags.pm | 108 +++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu/CPUFlags.pm b/src/PVE/API2/Qemu/CPUFlags.pm
index 672bd2d2..9baf6c3e 100644
--- a/src/PVE/API2/Qemu/CPUFlags.pm
+++ b/src/PVE/API2/Qemu/CPUFlags.pm
@@ -10,6 +10,9 @@ use PVE::QemuServer::CPUConfig;
 
 use base qw(PVE::RESTHandler);
 
+# vmx/svm are already represented by the nested-virt synthetic entry
+my %NESTED_VIRT_ALIASES = map { $_ => 1 } qw(vmx svm);
+
 __PACKAGE__->register_method({
     name => 'index',
     path => '',
@@ -21,6 +24,13 @@ __PACKAGE__->register_method({
         properties => {
             node => get_standard_option('pve-node'),
             arch => get_standard_option('pve-qm-cpu-arch', { optional => 1 }),
+            accel => {
+                description =>
+                    'Virtualization type to filter flags by. If not provided, return all flags.',
+                type => 'string',
+                enum => [qw(kvm tcg)],
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -35,6 +45,13 @@ __PACKAGE__->register_method({
                 description => {
                     type => 'string',
                     description => "Description of the CPU flag.",
+                    optional => 1,
+                },
+                'supported-on' => {
+                    description => 'List of nodes supporting the flag.',
+                    type => 'array',
+                    items => get_standard_option('pve-node'),
+                    optional => 1,
                 },
             },
         },
@@ -42,10 +59,99 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
+        my $accel = extract_param($param, 'accel');
         my $arch = extract_param($param, 'arch');
 
-        return PVE::QemuServer::CPUConfig::get_supported_cpu_flags($arch);
+        if (defined($arch) && $arch eq 'aarch64') {
+            return [];
+        }
+
+        my $descriptions = PVE::QemuServer::CPUConfig::description_by_flag($arch);
+
+        my $nested_virt = {
+            name => 'nested-virt',
+            description => $descriptions->{'nested-virt'},
+        };
+
+        return [$nested_virt, @{ extract_flags($descriptions, $accel) }];
     },
 });
 
+# As described here [0], in order to get an accurate picture of which flags can actually be used, we need
+# to intersect:
+#
+# 1. The understood CPU flags, i.e., all flags QEMU accepts in theory, but that may not be actually
+#   supported by the host CPU, and
+# 2. The supported CPU flags, which returns some settings/flags that cannot be used as `-cpu` arguments.
+#
+# [0] https://git.proxmox.com/?p=qemu-server.git;a=blob;f=src/PVE/QemuServer.pm;h=09e7a19b2f11ef48d2cfc11837b70338c306817c;hb=refs/heads/master#l2916
+sub extract_flags($descriptions, $accel = undef) {
+    my $recognized = extract_understood($descriptions);
+    my $supported = extract_supported($descriptions, $accel);
+
+    my %recognized_set = map { $_->{name} => 1 } @$recognized;
+
+    return [
+        map { {
+            name => $_->{name},
+            'supported-on' => $_->{'supported-on'},
+            (defined($_->{description}) ? (description => $_->{description}) : ()),
+        } } grep {
+            $recognized_set{ $_->{name} }
+        } @$supported
+    ];
+}
+
+sub extract_understood($descriptions) {
+    my $understood_cpu_flags = PVE::QemuServer::query_understood_cpu_flags();
+
+    return [
+        map {
+            my $entry = { name => $_ };
+            $entry->{description} = $descriptions->{$_} if $descriptions->{$_};
+            $entry;
+        } grep {
+            !$NESTED_VIRT_ALIASES{$_}
+        } @$understood_cpu_flags
+    ];
+}
+
+# We do not use `PVE::QemuServer::CPUConfig::query_supported_cpu_flags`, which is quite expensive since
+# it needs to spawn QEMU instances in order to check which flags are supported. Rather, we use its cached
+# output, which is stored by `pvestatd` [0].
+#
+# [0] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Service/pvestatd.pm;h=d0719446e3b9a5f1bd3c861dbe768432cb3d7a0e;hb=refs/heads/master#l87
+sub extract_supported($descriptions, $accel = undef) {
+    my %hash;
+
+    my sub add_flags($flags_by_node) {
+        for my $node (keys %$flags_by_node) {
+            # This depends on `pvestatd` storing the flags in space-separated format, which is the case
+            # at the time of this commit.
+            for (split(' ', $flags_by_node->{$node})) {
+                if ($hash{$_}) {
+                    $hash{$_}->{'supported-on'}->{$node} = 1;
+                } else {
+                    $hash{$_} = { 'supported-on' => { $node => 1 }, name => $_ };
+                }
+            }
+        }
+    }
+
+    add_flags(PVE::Cluster::get_node_kv('cpuflags-kvm')) if !defined($accel) || $accel eq 'kvm';
+    add_flags(PVE::Cluster::get_node_kv('cpuflags-tcg')) if !defined($accel) || $accel eq 'tcg';
+
+    return [
+        map {
+            my $entry = { %$_, 'supported-on' => [sort keys %{ $_->{'supported-on'} }] };
+            $entry->{description} = $descriptions->{ $_->{name} }
+                if $descriptions->{ $_->{name} };
+            $entry;
+        } sort {
+            $a->{name} cmp $b->{name}
+        } grep {
+            !$NESTED_VIRT_ALIASES{ $_->{name} }
+        } values %hash
+    ];
+}
 1;
-- 
2.47.3

[PATCH qemu-server 8/8] api: qemu: Add CRUD handlers for custom CPU models

[Arthur Bied-Charreton]

Add GET handlers for both all custom CPU models and specific ones, POST
handler for creating custom CPU models, PUT handler for updating them,
and DELETE handler for deleting them.

Original patches:
https://lore.proxmox.com/pve-devel/20211028114150.3245864-4-s.reiter@proxmox.com/
https://lore.proxmox.com/pve-devel/20211028114150.3245864-5-s.reiter@proxmox.com/

Originally-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Qemu/CPU.pm | 236 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 235 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu/CPU.pm b/src/PVE/API2/Qemu/CPU.pm
index f8a7e11d..9d89504d 100644
--- a/src/PVE/API2/Qemu/CPU.pm
+++ b/src/PVE/API2/Qemu/CPU.pm
@@ -52,7 +52,7 @@ __PACKAGE__->register_method({
                 },
             },
         },
-        links => [{ rel => 'child', href => '{name}' }],
+        links => [{ rel => 'child', href => 'model/{cputype}' }],
     },
     code => sub {
         my ($param) = @_;
@@ -67,4 +67,238 @@ __PACKAGE__->register_method({
     },
 });
 
+__PACKAGE__->register_method({
+    name => 'config',
+    path => 'model',
+    method => 'GET',
+    description => 'Read all custom CPU models definitions',
+    permissions => {
+        check => ['perm', '/nodes', ['Sys.Audit']],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            node => get_standard_option('pve-node'),
+        },
+    },
+    returns => {
+        type => 'array',
+        items => {
+            type => 'object',
+            properties => get_standard_option('pve-qemu-cpu'),
+        },
+    },
+    code => sub {
+        my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
+        delete $conf->{order};
+        my $ids = [];
+        foreach my $id (keys %{ $conf->{ids} }) {
+            delete $conf->{ids}->{$id}->{type};
+            push @$ids, $conf->{ids}->{$id};
+        }
+        return $ids;
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'create',
+    path => 'model',
+    method => 'POST',
+    protected => 1,
+    description => 'Add a custom CPU model definition',
+    permissions => {
+        check => ['perm', '/nodes', ['Sys.Console']],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
+            digest => get_standard_option('pve-config-digest'),
+            node => get_standard_option('pve-node'),
+        }),
+    },
+    returns => { type => 'null' },
+    code => sub {
+        my ($param) = @_;
+        delete $param->{node};
+
+        my $digest = extract_param($param, 'digest');
+
+        my $name = $param->{cputype};
+        die "custom cpu models 'cputype' must start with 'custom-' prefix\n"
+            if $name !~ m/^custom-/;
+        (my $name_no_prefix = $name) =~ s/^custom-//;
+
+        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
+            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
+            PVE::SectionConfig::assert_if_modified($conf, $digest);
+
+            die "custom CPU model '$name' already exists\n"
+                if defined($conf->{ids}->{$name_no_prefix});
+            $conf->{ids}->{$name_no_prefix} = $param;
+
+            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
+        });
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'delete',
+    path => 'model/{cputype}',
+    method => 'DELETE',
+    protected => 1,
+    description => 'Delete a custom CPU model definition',
+    permissions => {
+        check => ['perm', '/nodes', ['Sys.Console']],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            node => get_standard_option('pve-node'),
+            digest => get_standard_option('pve-config-digest'),
+            cputype => {
+                type => 'string',
+                description => "The custom model to delete. Must be prefixed with 'custom-'",
+            },
+        },
+    },
+    returns => { type => 'null' },
+    code => sub {
+        my ($param) = @_;
+
+        my $digest = extract_param($param, 'digest');
+
+        my $name = $param->{cputype};
+        die "'cputype' must start with 'custom-' prefix\n" if $name !~ m/^custom-/;
+        (my $name_no_prefix = $name) =~ s/^custom-//;
+
+        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
+            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
+            PVE::SectionConfig::assert_if_modified($conf, $digest);
+
+            die "custom CPU model '$name_no_prefix' does not exist\n"
+                if !defined($conf->{ids}->{$name_no_prefix});
+            delete $conf->{ids}->{$name_no_prefix};
+
+            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
+        });
+
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'update',
+    path => 'model/{cputype}',
+    method => 'PUT',
+    protected => 1,
+    description => "Update a custom CPU model definition.",
+    permissions => {
+        check => ['perm', '/nodes', ['Sys.Console']],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
+            node => get_standard_option('pve-node'),
+            digest => get_standard_option('pve-config-digest'),
+            delete => {
+                type => 'string',
+                format => 'pve-configid-list',
+                description => "A list of properties to delete.",
+                optional => 1,
+            },
+        }),
+    },
+    returns => { type => 'null' },
+    code => sub {
+        my ($param) = @_;
+        delete $param->{node};
+
+        my $digest = extract_param($param, 'digest');
+        my $delete = extract_param($param, 'delete') // '';
+        my %delete_hash = map { $_ => 1 } PVE::Tools::split_list($delete);
+
+        my $name = $param->{cputype};
+        die "custom CPU model's 'cputype' must start with 'custom-' prefix\n"
+            if $name !~ m/^custom-/;
+
+        (my $name_no_prefix = $name) =~ s/^custom-//;
+
+        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
+            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
+
+            PVE::SectionConfig::assert_if_modified($conf, $digest);
+
+            my $model = $conf->{ids}->{$name_no_prefix};
+            die "custom CPU model '$name_no_prefix' does not exist\n" if !defined($model);
+
+            my $props = PVE::QemuServer::CPUConfig::add_cpu_json_properties({});
+            for my $p (keys %$props) {
+                if ($delete_hash{$p}) {
+                    die "cannot delete 'cputype' property\n" if $p eq 'cputype';
+                    die "cannot set and delete property '$p' at once\n" if $param->{$p};
+                    delete $model->{$p};
+                } elsif (defined($param->{$p})) {
+                    $model->{$p} = $param->{$p};
+                }
+            }
+
+            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
+        });
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'info',
+    path => 'model/{cputype}',
+    method => 'GET',
+    description => 'Retrieve details about a specific CPU model',
+    permissions => {
+        check => ['perm', '/nodes', ['Sys.Audit']],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            node => get_standard_option('pve-node'),
+            cputype => {
+                type => 'string',
+                description =>
+                    "Name of the CPU model to query. Prefix with 'custom-' for custom models.",
+            },
+        },
+    },
+    returns => {
+        type => 'object',
+        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
+            vendor => {
+                type => 'string',
+                description => 'The CPU vendor reported to the guest OS. Only'
+                    . ' relevant for default models.',
+                optional => 1,
+            },
+            digest => get_standard_option('pve-config-digest'),
+        }),
+    },
+    code => sub {
+        my ($param) = @_;
+        my $cputype = $param->{cputype};
+
+        if ($cputype =~ m/^custom-/) {
+            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
+            my $digest = $conf->{digest};
+            my $retval = PVE::QemuServer::CPUConfig::get_custom_model($cputype);
+            $retval->{digest} = $digest;
+            return $retval;
+        }
+
+        # this correctly errors in case $cputype is unknown
+        my $vendor = PVE::QemuServer::CPUConfig::get_cpu_vendor($cputype);
+
+        my $retval = {
+            cputype => $cputype,
+            vendor => $vendor,
+        };
+
+        return $retval;
+    },
+});
+
 1;
-- 
2.47.3

Re: [PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints

[Fiona Ebner]

Nit: the qemu-server patches should come first, since the UI patches
depend on them. And it's always good to mention in the cover letter
which inter-package dependencies there are, i.e. pve-manager needs a
versioned dependency bump for qemu-server. It's quite obvious in this
case, but still helps and mostly writing this for the future.

The prefix 'qemu' doesn't really add any information here. It should
rather be something like 'cpu config:'

Am 12.03.26 um 9:39 AM schrieb Arthur Bied-Charreton:
> Add functionality to lock & write the custom CPU config and some other
> helpers that will be needed in custom CPU models CRUD endpoints.
> 
> Original patch:
> https://lore.proxmox.com/pve-devel/20211028114150.3245864-5-s.reiter@proxmox.com/
> 
> Originally-by: Stefan Reiter <s.reiter@proxmox.com>

If you want, you can put a short high-level changelog here to document
how the patch changed, e.g.:

[AB: split patch into adding helpers and adding API endpoints
     some other change]

This can help follow history better later on.

> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/QemuServer/CPUConfig.pm | 35 ++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
> index 32ec4954..2b6665a7 100644
> --- a/src/PVE/QemuServer/CPUConfig.pm
> +++ b/src/PVE/QemuServer/CPUConfig.pm
> @@ -6,10 +6,10 @@ use warnings;
>  use JSON;
>  
>  use PVE::JSONSchema qw(json_bool);
> -use PVE::Cluster qw(cfs_register_file cfs_read_file);
> +use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::ProcFSTools;
>  use PVE::RESTEnvironment qw(log_warn);
> -use PVE::Tools qw(run_command);
> +use PVE::Tools qw(run_command lock_file);
>  
>  use PVE::QemuServer::Helpers qw(min_version);
>  
> @@ -51,6 +51,19 @@ sub load_custom_model_conf {
>      return cfs_read_file($default_filename);
>  }
>  
> +sub write_custom_model_conf {
> +    my ($conf) = @_;
> +    cfs_write_file($default_filename, $conf);

Pre-existing, but could you add a preparatory commit renaming the
variable $default_filename to something more telling? Maybe
$cpu_models_filename?

> +}
> +
> +sub lock_cpu_config {
> +    my ($code) = @_;
> +    cfs_lock_file($default_filename, undef, $code);
> +    if (my $err = $@) {
> +        die $err;
> +    }

Style nit: could also just be
die $@ if $@;

> +}
> +
>  #builtin models : reported-model is mandatory
>  my $builtin_models_by_arch = {
>      x86_64 => {
> @@ -298,6 +311,19 @@ sub get_supported_cpu_flags {
>      return $supported_cpu_flags_by_arch->{$arch};
>  }
>  
> +sub description_by_flag {
> +    my ($arch) = @_;
> +    return { map { $_->{name} => $_->{description} } get_supported_cpu_flags($arch)->@* };
> +}
> +
> +sub get_cpu_vendor {
> +    my ($cputype, $arch) = @_;
> +    $arch = $host_arch if !defined($arch);

The $host_arch variable was recently dropped, needs a rebase.

> +    my $retval = $cpu_models_by_arch->{$arch}->{$cputype}
> +        or die "Built-in CPU type '$cputype' does not exist";
> +    return $retval;
> +}

These are quite small helpers and I do not see a big reason why they
should be added in a separate commit rather than just directly in the
commits with their users. It can be okay to add helpers up-front,
depending on the scenario, but here:
* 2 of the helpers are for config file handling
* 2 of the helpers are for cpu model property details, but also quite
unrelated
* the helpers are not complex

So I don't see enough semantic grouping that would justify having this
as a separate patch.

> +
>  my $all_supported_cpu_flags = {};
>  for my $arch ($supported_cpu_flags_by_arch->%*) {
>      for my $flag ($supported_cpu_flags_by_arch->{$arch}->@*) {
> @@ -375,6 +401,7 @@ my $cpu_fmt = {
>          optional => 1,
>      },
>  };
> +PVE::JSONSchema::register_standard_option('pve-qemu-cpu', $cpu_fmt);

I'd rather have this as a separate commit or squashed into the commit
with its first user.

>  
>  my $sev_fmt = {
>      type => {
> @@ -564,6 +591,7 @@ sub write_config {
>  
>          # saved in section header
>          delete $model_conf->{cputype};
> +        $model_conf->{type} = $class->type();

The commit message should at least describe why this is necessary. I
suppose for writing to actually work? Could also be a separate fix then,
but its fine to add together with the helper.

>      }
>  
>      $class->SUPER::write_config($filename, $cfg);
> @@ -612,7 +640,8 @@ sub get_cpu_models {
>  
>      my $conf = load_custom_model_conf();
>      for my $custom_model (keys %{ $conf->{ids} }) {
> -        my $reported_model = $conf->{ids}->{$custom_model}->{'reported-model'};
> +        my $model = $conf->{ids}->{$custom_model};
> +        my $reported_model = $model->{'reported-model'};

This change is unrelated to the rest.

>          $reported_model //= $cpu_fmt->{'reported-model'}->{default};
>          my $vendor = $all_cpu_models->{$reported_model};
>          push @$models,

Re: [PATCH qemu-server 7/8] api: qemu: Extend cpu-flags endpoint to return actually supported flags

[Fiona Ebner]

Similar comment regarding the 'qemu' prefix as in patch 6/8

Note that I'm not finished with the review and will continue next week
with patch 8/8 and a quick look at the UI patches. Still sending this
already for discussion.

Am 12.03.26 um 9:40 AM schrieb Arthur Bied-Charreton:
> Previously the endpoint returned a hardcoded list of flags. It now
> returns flags that are both recognized by QEMU, and supported by at
> least one cluster node to give a somewhat accurate picture of what flags
> can actually be used on the current cluster.
> 
> The 'nested-virt` entry is prepended. An optional 'accel' parameter
> filters results by virtualization type (kvm or tcg) to help avoid
> misconfigurations when assigning flags to VMs with a specific
> acceleration setting.
> 
> This deviates from the original patch [0] by adding the functionality to
> the `cpu-flags` endpoint instead of adding new endpoints. This is
> because we never need the understood/supported flags alone in the
> frontend, only their intersection. This also improves the VM CPU flag
> selector by letting users select from all possible flags in their
> cluster.
> 
> When passed `aarch64` as argument for `arch`, the index returns an empty
> list, which is consistent with the behavior from before this patch.
> 
> [0]
> https://lore.proxmox.com/pve-devel/20211028114150.3245864-3-s.reiter@proxmox.com/
> 
> Originally-by: Stefan Reiter <s.reiter@proxmox.com>

I don't think there is enough left of the original patch/approach in
this case to keep that trailer.

> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/API2/Qemu/CPUFlags.pm | 108 +++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/Qemu/CPUFlags.pm b/src/PVE/API2/Qemu/CPUFlags.pm
> index 672bd2d2..9baf6c3e 100644
> --- a/src/PVE/API2/Qemu/CPUFlags.pm
> +++ b/src/PVE/API2/Qemu/CPUFlags.pm
> @@ -10,6 +10,9 @@ use PVE::QemuServer::CPUConfig;

Missing use statements for PVE::QemuServer and PVE::Cluster

>  
>  use base qw(PVE::RESTHandler);
>  
> +# vmx/svm are already represented by the nested-virt synthetic entry
> +my %NESTED_VIRT_ALIASES = map { $_ => 1 } qw(vmx svm);

I'd prefer a short helper function with a bit more generic name, like
flag_is_aliased(). And/or maybe have a filter_cpu_flags() function.

> +
>  __PACKAGE__->register_method({
>      name => 'index',
>      path => '',
> @@ -21,6 +24,13 @@ __PACKAGE__->register_method({
>          properties => {
>              node => get_standard_option('pve-node'),
>              arch => get_standard_option('pve-qm-cpu-arch', { optional => 1 }),
> +            accel => {
> +                description =>
> +                    'Virtualization type to filter flags by. If not provided, return all flags.',
> +                type => 'string',
> +                enum => [qw(kvm tcg)],
> +                optional => 1,
> +            },
>          },
>      },
>      returns => {
> @@ -35,6 +45,13 @@ __PACKAGE__->register_method({
>                  description => {
>                      type => 'string',
>                      description => "Description of the CPU flag.",
> +                    optional => 1,
> +                },
> +                'supported-on' => {
> +                    description => 'List of nodes supporting the flag.',
> +                    type => 'array',
> +                    items => get_standard_option('pve-node'),
> +                    optional => 1,
>                  },
>              },
>          },
> @@ -42,10 +59,99 @@ __PACKAGE__->register_method({
>      code => sub {
>          my ($param) = @_;
>  
> +        my $accel = extract_param($param, 'accel');
>          my $arch = extract_param($param, 'arch');
>  
> -        return PVE::QemuServer::CPUConfig::get_supported_cpu_flags($arch);

NAK: this endpoint is used for the VM-specific selection right now and
you can only configure flags that are allow-listed for a VM-specific CPU
model, i.e. the get_supported_cpu_flags() flags, not all flags. Some
flags do have security implications for the host too, that's why
creating custom models with arbitrary flags must be a more privileged
operation. I also think it would be a bit overkill to have all flags
there, rather than the ones we deem most useful to expose.

So the API call should not start providing other flags by default,
because the non-allow-listed flags cannot be set anyways for a
VM-specific model. We can add a new API parameter for this, maybe
"kind/type/something-else?" being an enum with values 'all|vm-specific'
and by default having 'vm-specific'.

I don't think there ever is a case where we want all flags regardless of
accelerator, or is there? If not, we should make 'accel' have a default
value to avoid the case where it's not provided at all being treated
differently. I'd opt for 'kvm' as that's the more common use case.

> +        if (defined($arch) && $arch eq 'aarch64') {
> +            return [];

We should document this in the description and add a TODO comment for
the 'all flags' case. Not having any VM-specific flags is intentional
right now. Not having any usable flags is not. It's just that we don't
ship a list yet, as it's more difficult to obtain during QEMU build. For
x86_64, kvm -cpu help will list the flags.

> +        }
> +
> +        my $descriptions = PVE::QemuServer::CPUConfig::description_by_flag($arch);

We could avoid passing around the descriptions, by having a function
get_flag_description() and call that where needed. We can also have
CPUConfig cache it with a hash to avoid re-grepping every time. That
would feel a bit cleaner to me.

> +
> +        my $nested_virt = {
> +            name => 'nested-virt',
> +            description => $descriptions->{'nested-virt'},

Should we check which hosts do support it? I.e. check for svm|vmx and
declare support if either is present on a host.

> +        };
> +
> +        return [$nested_virt, @{ extract_flags($descriptions, $accel) }];
>      },
>  });
>  
> +# As described here [0], in order to get an accurate picture of which flags can actually be used, we need
> +# to intersect:
> +#
> +# 1. The understood CPU flags, i.e., all flags QEMU accepts in theory, but that may not be actually
> +#   supported by the host CPU, and
> +# 2. The supported CPU flags, which returns some settings/flags that cannot be used as `-cpu` arguments.
> +#
> +# [0] https://git.proxmox.com/?p=qemu-server.git;a=blob;f=src/PVE/QemuServer.pm;h=09e7a19b2f11ef48d2cfc11837b70338c306817c;hb=refs/heads/master#l2916

I'd prefer having "See the documentation of query_supported_cpu_flags()
and query_understood_cpu_flags() for more information." rather than
putting a web link. Then the reference stays up-to-date with any changes
there.

> +sub extract_flags($descriptions, $accel = undef) {

To me, the 'extract' in the function names here and for the others below
is rather confusing. The functions take the hash of supported flags
(currently called $descriptions) and adds more information/other flags.
The only thing it's extracting is the descriptions.

> +    my $recognized = extract_understood($descriptions);
> +    my $supported = extract_supported($descriptions, $accel);
> +
> +    my %recognized_set = map { $_->{name} => 1 } @$recognized;
> +
> +    return [
> +        map { {
> +            name => $_->{name},
> +            'supported-on' => $_->{'supported-on'},
> +            (defined($_->{description}) ? (description => $_->{description}) : ()),
> +        } } grep {
> +            $recognized_set{ $_->{name} }
> +        } @$supported
> +    ];
> +}
> +
> +sub extract_understood($descriptions) {
> +    my $understood_cpu_flags = PVE::QemuServer::query_understood_cpu_flags();
> +
> +    return [
> +        map {
> +            my $entry = { name => $_ };
> +            $entry->{description} = $descriptions->{$_} if $descriptions->{$_};
> +            $entry;
> +        } grep {
> +            !$NESTED_VIRT_ALIASES{$_}
> +        } @$understood_cpu_flags
> +    ];
> +}
> +
> +# We do not use `PVE::QemuServer::CPUConfig::query_supported_cpu_flags`, which is quite expensive since
> +# it needs to spawn QEMU instances in order to check which flags are supported. Rather, we use its cached
> +# output, which is stored by `pvestatd` [0].
> +#
> +# [0] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Service/pvestatd.pm;h=d0719446e3b9a5f1bd3c861dbe768432cb3d7a0e;hb=refs/heads/master#l87

Again, not a fan of putting a web reference here. I think it's enough to
mention that there are entries in the node kv store (regularly updated
by pvestatd).

> +sub extract_supported($descriptions, $accel = undef) {
> +    my %hash;
> +
> +    my sub add_flags($flags_by_node) {
> +        for my $node (keys %$flags_by_node) {
> +            # This depends on `pvestatd` storing the flags in space-separated format, which is the case
> +            # at the time of this commit.
> +            for (split(' ', $flags_by_node->{$node})) {
> +                if ($hash{$_}) {
> +                    $hash{$_}->{'supported-on'}->{$node} = 1;
> +                } else {
> +                    $hash{$_} = { 'supported-on' => { $node => 1 }, name => $_ };
> +                }
> +            }
> +        }
> +    }

I didn't know you could do nested subs directly and not just by
assigning a closure to a local variable. I'm not sure we have this
anywhere else in the code base and if there are any gotchas when doing
it like this.

@Wolfgang: anything to be aware of with this construct?

> +
> +    add_flags(PVE::Cluster::get_node_kv('cpuflags-kvm')) if !defined($accel) || $accel eq 'kvm';
> +    add_flags(PVE::Cluster::get_node_kv('cpuflags-tcg')) if !defined($accel) || $accel eq 'tcg';
> +
> +    return [
> +        map {
> +            my $entry = { %$_, 'supported-on' => [sort keys %{ $_->{'supported-on'} }] };
> +            $entry->{description} = $descriptions->{ $_->{name} }
> +                if $descriptions->{ $_->{name} };
> +            $entry;
> +        } sort {
> +            $a->{name} cmp $b->{name}
> +        } grep {
> +            !$NESTED_VIRT_ALIASES{ $_->{name} }
> +        } values %hash

Style nit: we usually write such things in a more mixed way rather than
purely functional. One advantage is that one doesn't have to read
bottom-to-top. Opinionated preference, but I think when you have
multiple instructions inside a map expression, it is a good sign that
it's better written as a loop.

> +    ];
> +}
>  1;