[pve-devel] [PATCH qemu-server docs manager] Implement support for fw_cfg

[pve-devel] [PATCH qemu-server docs manager] Implement support for fw_cfg

[Leo Nunner]

This patch introduces an interface for passing values to a guest via the
fw_cfg parameter. Settings are in the format

    opt/$key=$value

Where $key should start with a rfqdn (but doesn't need to). Both plain
strings and files (only from the snippets directory) can be passed through.
An example application for this parameter is for provisioning CoreOS
systems [1].

Files are currently passed through if the user specifies the value as
"storage:snippets/file", there is no implicit file flag.

[1] https://coreos.github.io/ignition/

qemu-server:

Leo Nunner (2):
  fix #4068: implement support for fw_cfg
  test: add cfg2cmd tests for fw_cfg

 PVE/API2/Qemu.pm                     | 14 +++++++++++
 PVE/QemuServer.pm                    | 35 ++++++++++++++++++++++++++++
 test/cfg2cmd/fw_cfg-files.conf       | 15 ++++++++++++
 test/cfg2cmd/fw_cfg-files.conf.cmd   | 30 ++++++++++++++++++++++++
 test/cfg2cmd/fw_cfg-strings.conf     | 15 ++++++++++++
 test/cfg2cmd/fw_cfg-strings.conf.cmd | 30 ++++++++++++++++++++++++
 6 files changed, 139 insertions(+)
 create mode 100644 test/cfg2cmd/fw_cfg-files.conf
 create mode 100644 test/cfg2cmd/fw_cfg-files.conf.cmd
 create mode 100644 test/cfg2cmd/fw_cfg-strings.conf
 create mode 100644 test/cfg2cmd/fw_cfg-strings.conf.cmd

docs:

Leo Nunner (1):
  fix #4068: document fw_cfg parameter

 qm.adoc | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

manager:

Leo Nunner (1):
  fix #4068: expose fw_cfg through the GUI

 www/manager6/Makefile                |   1 +
 www/manager6/qemu/FirmwareCfgEdit.js | 224 +++++++++++++++++++++++++++
 www/manager6/qemu/Options.js         |   6 +
 3 files changed, 231 insertions(+)
 create mode 100644 www/manager6/qemu/FirmwareCfgEdit.js

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/2] fix #4068: implement support for fw_cfg

[Leo Nunner]

Implements support for passing values to the fw_cfg argument for QEMU.
If the value looks like a file, the backend checks for the correct
permissions/path and if everything is right, includes it as a file
instead of as a string.

Setting the argument requires the VM.Config.Options permission on the
guest. Including files requires Datastore.Audit and Datastore.Allocate
on the specific storage.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
RFC: I feel like a more implicit option for passing files would be
nicer, but I can't really think of a nice way…

 PVE/API2/Qemu.pm  | 14 ++++++++++++++
 PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 587bb22..c03394f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub {
 	    # the user needs Disk and PowerMgmt privileges to change the vmstate
 	    # also needs privileges on the storage, that will be checked later
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', 'VM.PowerMgmt' ]);
+	} elsif ($opt eq 'fw_cfg') {
+	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
 	} else {
 	    # catches hostpci\d+, args, lock, etc.
 	    # new options will be checked here
@@ -1770,6 +1772,18 @@ my $update_vm_api  = sub {
 		} elsif ($opt eq 'tags') {
 		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
 		    $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt});
+		} elsif ($opt eq 'fw_cfg') {
+		    foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) {
+			my ($opt, $val) = split("=", $fw_cfg);
+
+			if (my $storage = PVE::Storage::parse_volume_id($val, 1)) {
+			    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.Audit', 'Datastore.Allocate']);
+
+			    my ($path, undef, $type) = PVE::Storage::path($storecfg, $val);
+			    die "File $val is not in snippets directory\n" if $type ne "snippets";
+			}
+		    }
+		    $conf->{pending}->{$opt} = $param->{$opt};
 		} else {
 		    $conf->{pending}->{$opt} = $param->{$opt};
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 40be44d..c5dea1f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -723,6 +723,12 @@ EODESCR
 	description => "List of host cores used to execute guest processes, for example: 0,5,8-11",
 	optional => 1,
     },
+    fw_cfg => {
+	type => 'string',
+	optional => 1,
+	description => 'Pass values to the guest via the fw_cfg parameter.',
+	format => 'pve-fw-cfg-list',
+    },
 };
 
 my $cicustom_fmt = {
@@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path {
     return $volid;
 }
 
+PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg);
+sub verify_fw_cfg {
+    my ($value, $noerr) = @_;
+
+    my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/;
+
+    if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) {
+	return $value;
+    }
+
+    return if $noerr;
+
+    die "unable to parse fw_cfg option\n";
+}
+
 my $usb_fmt = {
     host => {
 	default_key => 1,
@@ -4181,6 +4202,20 @@ sub config_to_command {
 	push @$cmd, '-snapshot';
     }
 
+    if ($conf->{fw_cfg}) {
+	foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) {
+	    my ($opt, $val) = split("=", $conf);
+
+	    push @$cmd, "-fw_cfg";
+	    if (PVE::Storage::parse_volume_id($val, 1)) {
+		my $path = PVE::Storage::path($storecfg, $val);
+		push @$cmd, "$opt,file=$path";
+	    } else {
+		push @$cmd, "$opt,string=$val";
+	    }
+	}
+    }
+
     # add custom args
     if ($conf->{args}) {
 	my $aa = PVE::Tools::split_args($conf->{args});
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] test: add cfg2cmd tests for fw_cfg

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 test/cfg2cmd/fw_cfg-files.conf       | 15 ++++++++++++++
 test/cfg2cmd/fw_cfg-files.conf.cmd   | 30 ++++++++++++++++++++++++++++
 test/cfg2cmd/fw_cfg-strings.conf     | 15 ++++++++++++++
 test/cfg2cmd/fw_cfg-strings.conf.cmd | 30 ++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+)
 create mode 100644 test/cfg2cmd/fw_cfg-files.conf
 create mode 100644 test/cfg2cmd/fw_cfg-files.conf.cmd
 create mode 100644 test/cfg2cmd/fw_cfg-strings.conf
 create mode 100644 test/cfg2cmd/fw_cfg-strings.conf.cmd

diff --git a/test/cfg2cmd/fw_cfg-files.conf b/test/cfg2cmd/fw_cfg-files.conf
new file mode 100644
index 0000000..a8117d1
--- /dev/null
+++ b/test/cfg2cmd/fw_cfg-files.conf
@@ -0,0 +1,15 @@
+# TEST: Config with file values for fw_cfg
+# QEMU_VERSION: 7.0.0
+bios: ovmf
+bootdisk: scsi0
+cores: 2
+efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
+fw_cfg: opt/com.proxmox/test1=local:snippets/test1,opt/com.proxmox/test2=cifs-store:snippets/test2
+machine: q35
+meta: creation-qemu=6.1
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/fw_cfg-files.conf.cmd b/test/cfg2cmd/fw_cfg-files.conf.cmd
new file mode 100644
index 0000000..7d14053
--- /dev/null
+++ b/test/cfg2cmd/fw_cfg-files.conf.cmd
@@ -0,0 +1,30 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
+  -drive 'if=pflash,unit=1,id=drive-efidisk0,format=qcow2,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
+  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
+  -machine 'type=q35+pve0' \
+  -fw_cfg 'opt/com.proxmox/test1,file=/var/lib/vz/snippets/test1' \
+  -fw_cfg 'opt/com.proxmox/test2,file=/mnt/pve/cifs-store/snippets/test2'
diff --git a/test/cfg2cmd/fw_cfg-strings.conf b/test/cfg2cmd/fw_cfg-strings.conf
new file mode 100644
index 0000000..1d3631b
--- /dev/null
+++ b/test/cfg2cmd/fw_cfg-strings.conf
@@ -0,0 +1,15 @@
+# TEST: Config with simple string values for fw_cfg
+# QEMU_VERSION: 7.0.0
+bios: ovmf
+bootdisk: scsi0
+cores: 2
+efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
+fw_cfg: opt/com.proxmox/test1=first,opt/com.proxmox/test2=second
+machine: q35
+meta: creation-qemu=6.1
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/fw_cfg-strings.conf.cmd b/test/cfg2cmd/fw_cfg-strings.conf.cmd
new file mode 100644
index 0000000..9ff7dc2
--- /dev/null
+++ b/test/cfg2cmd/fw_cfg-strings.conf.cmd
@@ -0,0 +1,30 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
+  -drive 'if=pflash,unit=1,id=drive-efidisk0,format=qcow2,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
+  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
+  -machine 'type=q35+pve0' \
+  -fw_cfg 'opt/com.proxmox/test1,string=first' \
+  -fw_cfg 'opt/com.proxmox/test2,string=second'
-- 
2.30.2

[pve-devel] [PATCH docs] fix #4068: document fw_cfg parameter

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 qm.adoc | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index bd535a2..0c587ad 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1139,6 +1139,24 @@ http://localhost:9843 in a browser in the guest.
 
 It can help to restart the SPICE session.
 
+[[qm_fw_cfg]]
+QEMU Firmware Configuration
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+QEMU allows passing values to the guest via the 'fw_cfg' parameter. Settings
+are defined on a key/value basis, where the key follows a directory-like structure:
+
+----
+opt/com.proxmox/test1=local:snippets/config.txt
+opt/com.proxmox/test2=foobar
+----
+
+These settings can be edited in the GUI via 'Options' -> 'QEMU Firmware Configuration'.
+User-supplied keys *must* be prefixed with `opt/` and are recommended to start with a
+reverse fully qualified domain name. `opt/ovmf/` and `opt/org.qemu/` are reserved for
+internal use by QEMU and should not be set. Setting a value in the format
+`storage:snippets/file` causes the contents of the file to be included for the key.
+
 [[qm_migration]]
 Migration
 ---------
-- 
2.30.2

[pve-devel] [PATCH manager] fix #4068: expose fw_cfg through the GUI

[Leo Nunner]

Introduces a new UI element to set/edit/delete any number of fw_cfg
arguments in a table-like manner.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 www/manager6/Makefile                |   1 +
 www/manager6/qemu/FirmwareCfgEdit.js | 224 +++++++++++++++++++++++++++
 www/manager6/qemu/Options.js         |   6 +
 3 files changed, 231 insertions(+)
 create mode 100644 www/manager6/qemu/FirmwareCfgEdit.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 05afeda4..bf3c8b20 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -217,6 +217,7 @@ JSSRC= 							\
 	qemu/Config.js					\
 	qemu/CreateWizard.js				\
 	qemu/DisplayEdit.js				\
+	qemu/FirmwareCfgEdit.js				\
 	qemu/HDEdit.js					\
 	qemu/HDEfi.js					\
 	qemu/HDTPM.js					\
diff --git a/www/manager6/qemu/FirmwareCfgEdit.js b/www/manager6/qemu/FirmwareCfgEdit.js
new file mode 100644
index 00000000..031042e3
--- /dev/null
+++ b/www/manager6/qemu/FirmwareCfgEdit.js
@@ -0,0 +1,224 @@
+Ext.define('pve-fw-cfg-option', {
+    extend: 'Ext.data.Model',
+    fields: [
+	{ name: 'opt', type: 'string' },
+	{ name: 'val', type: 'string' },
+    ],
+});
+
+Ext.define('PVE.qemu.FirmwareCfgPanel', {
+    extend: 'Ext.form.FieldContainer',
+    alias: 'widget.pveQemuFirmwareCfgPanel',
+
+    config: {}, // store loaded vm config
+    store: undefined,
+
+    inUpdate: false,
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	init: function(view) {
+	    let me = this;
+	    let grid = me.lookup('grid');
+
+	    view.store = Ext.create('Ext.data.Store', {
+		model: 'pve-fw-cfg-option',
+	    });
+
+	    grid.setStore(view.store);
+	},
+
+	addOption: function() {
+	    let me = this;
+	    me.lookup('grid').getStore().add({});
+	},
+
+	removeOption: function(field) {
+	    let me = this;
+	    let record = field.getWidgetRecord();
+	    me.lookup('grid').getStore().remove(record);
+	    me.setMarkerValue();
+	},
+
+	onUpdate: function(record, property, value) {
+	    let me = this;
+
+	    if (record === undefined) {
+		return;
+	    }
+
+	    record.set(property, value);
+	    record.commit();
+	    me.setMarkerValue();
+	},
+
+	onUpdateOption: function(field, value) {
+	    let me = this;
+	    let record = field.getWidgetRecord();
+
+	    me.onUpdate(record, "opt", value);
+	},
+
+	onUpdateValue: function(field, value) {
+	    let me = this;
+	    let record = field.getWidgetRecord();
+
+	    me.onUpdate(record, "val", value);
+	},
+
+	setMarkerValue() {
+	    let me = this;
+	    let view = me.getView();
+
+	    view.inUpdate = true;
+	    me.lookup('marker').setValue(view.calculateValue());
+	    view.inUpdate = false;
+	},
+
+	control: {
+	    "grid textfield[dest=opt]": {
+		change: "onUpdateOption",
+	    },
+	    "grid textfield[dest=val]": {
+		change: "onUpdateValue",
+	    },
+	},
+    },
+
+    loadConfig: function(config) {
+	let me = this;
+	let marker = me.lookup('marker');
+	let list = PVE.Parser.parsePropertyString(config.fw_cfg);
+	let options = [];
+
+	me.config = config;
+	me.store.removeAll();
+
+	for (const [key, value] of Object.entries(list)) {
+	    options.push({
+		opt: key,
+		val: value,
+	    });
+	}
+
+	marker.originalValue = config.fw_cfg;
+	marker.value = config.fw_cfg;
+	me.store.setData(options);
+    },
+
+    calculateValue: function() {
+	let me = this;
+	let ret = [];
+	me.store.each((record) => {
+	    ret.push(record.data.opt + "=" + record.data.val);
+	});
+	return ret.join(",");
+    },
+
+    items: [
+	{
+	    xtype: 'grid',
+	    reference: 'grid',
+	    margin: '0 0 5 0',
+	    height: 150,
+	    defaults: {
+		sortable: false,
+		hideable: false,
+		draggable: false,
+	    },
+	    columns: [
+		{
+		    xtype: 'widgetcolumn',
+		    text: gettext('Option'),
+		    dataIndex: 'opt',
+		    flex: 1,
+		    isFormField: false,
+		    widget: {
+			xtype: 'textfield',
+			allowBlank: false,
+			dest: 'opt',
+			emptyText: 'opt/...',
+		    },
+		},
+		{
+		    xtype: 'widgetcolumn',
+		    text: gettext('Value'),
+		    dataIndex: 'val',
+		    flex: 1,
+		    isFormField: false,
+		    widget: {
+			xtype: 'textfield',
+			allowBlank: false,
+			dest: 'val',
+		    },
+		},
+		{
+		    xtype: 'widgetcolumn',
+		    width: 40,
+		    widget: {
+			xtype: 'button',
+			iconCls: 'fa fa-trash-o',
+			handler: 'removeOption',
+		    },
+		},
+	    ],
+	},
+	{
+	    // for dirty marking and 'reset' function
+	    xtype: 'hiddenfield',
+	    reference: 'marker',
+	    name: 'fw_cfg',
+	    setValue: function(value) {
+		let me = this;
+		let panel = me.up('pveQemuFirmwareCfgPanel');
+
+		// Reset
+		if (!panel.inUpdate) {
+		    panel.loadConfig(panel.config);
+		}
+
+		me.value = value;
+		me.checkDirty();
+	    },
+	    getValue: function() {
+		return this.value;
+	    },
+	    getSubmitValue: function() {
+		return this.value;
+	    },
+	},
+	{
+	    xtype: 'button',
+	    text: gettext('Add'),
+	    iconCls: 'fa fa-plus-circle',
+	    handler: 'addOption',
+	},
+    ],
+});
+
+Ext.define('PVE.qemu.FirmwareCfgEdit', {
+    extend: 'Proxmox.window.Edit',
+
+    items: [{
+	xtype: 'pveQemuFirmwareCfgPanel',
+	itemId: 'inputpanel',
+    }],
+
+    subject: gettext('Firmware Configuration'),
+    onlineHelp: 'qm_fw_cfg',
+    width: 640,
+
+    getValues: function() {
+	let me = this;
+	let values = me.callParent();
+	return values.fw_cfg ? values : { 'delete': 'fw_cfg' };
+    },
+
+    initComponent: function() {
+	let me = this;
+	me.callParent();
+	me.load({
+	    success: ({ result }) => me.down('#inputpanel').loadConfig(result.data),
+	});
+    },
+});
diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
index 7b112400..80407010 100644
--- a/www/manager6/qemu/Options.js
+++ b/www/manager6/qemu/Options.js
@@ -338,6 +338,12 @@ Ext.define('PVE.qemu.Options', {
 		    },
 		} : undefined,
 	    },
+	    fw_cfg: {
+		header: gettext('QEMU Firmware Configuration'),
+		defaultValue: '',
+		renderer: val => val || Proxmox.Utils.noneText,
+		editor: caps.vms['VM.Config.Options'] ? 'PVE.qemu.FirmwareCfgEdit' : undefined,
+	    },
 	    hookscript: {
 		header: gettext('Hookscript'),
 	    },
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 1/2] fix #4068: implement support for fw_cfg

[Wolfgang Bumiller]

On Wed, Mar 01, 2023 at 10:12:26AM +0100, Leo Nunner wrote:
> Implements support for passing values to the fw_cfg argument for QEMU.
> If the value looks like a file, the backend checks for the correct
> permissions/path and if everything is right, includes it as a file
> instead of as a string.
> 
> Setting the argument requires the VM.Config.Options permission on the
> guest. Including files requires Datastore.Audit and Datastore.Allocate
> on the specific storage.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> RFC: I feel like a more implicit option for passing files would be
> nicer, but I can't really think of a nice way…
> 
>  PVE/API2/Qemu.pm  | 14 ++++++++++++++
>  PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..c03394f 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub {
>  	    # the user needs Disk and PowerMgmt privileges to change the vmstate
>  	    # also needs privileges on the storage, that will be checked later
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', 'VM.PowerMgmt' ]);
> +	} elsif ($opt eq 'fw_cfg') {
> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
>  	} else {
>  	    # catches hostpci\d+, args, lock, etc.
>  	    # new options will be checked here
> @@ -1770,6 +1772,18 @@ my $update_vm_api  = sub {
>  		} elsif ($opt eq 'tags') {
>  		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
>  		    $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt});
> +		} elsif ($opt eq 'fw_cfg') {
> +		    foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) {
> +			my ($opt, $val) = split("=", $fw_cfg);
> +
> +			if (my $storage = PVE::Storage::parse_volume_id($val, 1)) {
> +			    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.Audit', 'Datastore.Allocate']);
> +
> +			    my ($path, undef, $type) = PVE::Storage::path($storecfg, $val);
> +			    die "File $val is not in snippets directory\n" if $type ne "snippets";
> +			}
> +		    }
> +		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 40be44d..c5dea1f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -723,6 +723,12 @@ EODESCR
>  	description => "List of host cores used to execute guest processes, for example: 0,5,8-11",
>  	optional => 1,
>      },
> +    fw_cfg => {
> +	type => 'string',
> +	optional => 1,
> +	description => 'Pass values to the guest via the fw_cfg parameter.',
> +	format => 'pve-fw-cfg-list',
> +    },
>  };
>  
>  my $cicustom_fmt = {
> @@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path {
>      return $volid;
>  }
>  
> +PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg);
> +sub verify_fw_cfg {
> +    my ($value, $noerr) = @_;
> +
> +    my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/;

IMO this is way too restrictive.  If we pass the `name=` prefix in the
`-fw_cfg` parameter we can even allow `=`.  In fact, AFAICT only `,` is
problematic and would need to be doubled. Also if we really just have a
single option with a list... both `,` and `;` will be problematic on our
end, so it's fine to exclude those 2 for now. (Until we add quoting
support to split_list ;-) )

That said, for the use cases I have in mind for us, the current code
would be sufficient, but at least uppercase letters, underscores and
spaces would be nice...

> +
> +    if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) {
> +	return $value;
> +    }
> +
> +    return if $noerr;
> +
> +    die "unable to parse fw_cfg option\n";
> +}
> +
>  my $usb_fmt = {
>      host => {
>  	default_key => 1,
> @@ -4181,6 +4202,20 @@ sub config_to_command {
>  	push @$cmd, '-snapshot';
>      }
>  
> +    if ($conf->{fw_cfg}) {
> +	foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) {
> +	    my ($opt, $val) = split("=", $conf);
> +
> +	    push @$cmd, "-fw_cfg";
> +	    if (PVE::Storage::parse_volume_id($val, 1)) {
> +		my $path = PVE::Storage::path($storecfg, $val);
> +		push @$cmd, "$opt,file=$path";
> +	    } else {
> +		push @$cmd, "$opt,string=$val";

^ This would definitely be safer with the `name=` portion included.

Re: [pve-devel] [PATCH qemu-server 1/2] fix #4068: implement support for fw_cfg

[Leo Nunner]

Thanks for the review!

On 2023-03-15 15:05, Wolfgang Bumiller wrote:
> On Wed, Mar 01, 2023 at 10:12:26AM +0100, Leo Nunner wrote:
>> Implements support for passing values to the fw_cfg argument for QEMU.
>> If the value looks like a file, the backend checks for the correct
>> permissions/path and if everything is right, includes it as a file
>> instead of as a string.
>>
>> Setting the argument requires the VM.Config.Options permission on the
>> guest. Including files requires Datastore.Audit and Datastore.Allocate
>> on the specific storage.
>>
>> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
>> ---
>> RFC: I feel like a more implicit option for passing files would be
>> nicer, but I can't really think of a nice way…
>>
>>  PVE/API2/Qemu.pm  | 14 ++++++++++++++
>>  PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 587bb22..c03394f 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub {
>>  	    # the user needs Disk and PowerMgmt privileges to change the vmstate
>>  	    # also needs privileges on the storage, that will be checked later
>>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', 'VM.PowerMgmt' ]);
>> +	} elsif ($opt eq 'fw_cfg') {
>> +	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
>>  	} else {
>>  	    # catches hostpci\d+, args, lock, etc.
>>  	    # new options will be checked here
>> @@ -1770,6 +1772,18 @@ my $update_vm_api  = sub {
>>  		} elsif ($opt eq 'tags') {
>>  		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
>>  		    $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt});
>> +		} elsif ($opt eq 'fw_cfg') {
>> +		    foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) {
>> +			my ($opt, $val) = split("=", $fw_cfg);
>> +
>> +			if (my $storage = PVE::Storage::parse_volume_id($val, 1)) {
>> +			    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.Audit', 'Datastore.Allocate']);
>> +
>> +			    my ($path, undef, $type) = PVE::Storage::path($storecfg, $val);
>> +			    die "File $val is not in snippets directory\n" if $type ne "snippets";
>> +			}
>> +		    }
>> +		    $conf->{pending}->{$opt} = $param->{$opt};
>>  		} else {
>>  		    $conf->{pending}->{$opt} = $param->{$opt};
>>  
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 40be44d..c5dea1f 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -723,6 +723,12 @@ EODESCR
>>  	description => "List of host cores used to execute guest processes, for example: 0,5,8-11",
>>  	optional => 1,
>>      },
>> +    fw_cfg => {
>> +	type => 'string',
>> +	optional => 1,
>> +	description => 'Pass values to the guest via the fw_cfg parameter.',
>> +	format => 'pve-fw-cfg-list',
>> +    },
>>  };
>>  
>>  my $cicustom_fmt = {
>> @@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path {
>>      return $volid;
>>  }
>>  
>> +PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg);
>> +sub verify_fw_cfg {
>> +    my ($value, $noerr) = @_;
>> +
>> +    my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/;
> IMO this is way too restrictive.  If we pass the `name=` prefix in the
> `-fw_cfg` parameter we can even allow `=`.  In fact, AFAICT only `,` is
> problematic and would need to be doubled. Also if we really just have a
> single option with a list... both `,` and `;` will be problematic on our
> end, so it's fine to exclude those 2 for now. (Until we add quoting
> support to split_list ;-) )
>
> That said, for the use cases I have in mind for us, the current code
> would be sufficient, but at least uppercase letters, underscores and
> spaces would be nice...
We could also just switch to something like `[^,;]+`, or would that be
too extreme?
>> +
>> +    if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) {
>> +	return $value;
>> +    }
>> +
>> +    return if $noerr;
>> +
>> +    die "unable to parse fw_cfg option\n";
>> +}
>> +
>>  my $usb_fmt = {
>>      host => {
>>  	default_key => 1,
>> @@ -4181,6 +4202,20 @@ sub config_to_command {
>>  	push @$cmd, '-snapshot';
>>      }
>>  
>> +    if ($conf->{fw_cfg}) {
>> +	foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) {
>> +	    my ($opt, $val) = split("=", $conf);
>> +
>> +	    push @$cmd, "-fw_cfg";
>> +	    if (PVE::Storage::parse_volume_id($val, 1)) {
>> +		my $path = PVE::Storage::path($storecfg, $val);
>> +		push @$cmd, "$opt,file=$path";
>> +	    } else {
>> +		push @$cmd, "$opt,string=$val";
> ^ This would definitely be safer with the `name=` portion included.
Agreed, I'll change it accordingly.