Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
src/qemu/qemu_command.c | 18 ++++
src/qemu/qemu_domain.c | 84 +++++++++++++++++++
.../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
.../tseg-old-machine-type.args | 27 ++++++
.../tseg-old-machine-type.xml | 21 +++++
tests/qemuxml2argvdata/tseg.args | 28 +++++++
tests/qemuxml2argvdata/tseg.xml | 21 +++++
tests/qemuxml2argvtest.c | 48 +++++++++++
.../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
.../tseg-old-machine-type.xml | 44 ++++++++++
tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
tests/qemuxml2xmltest.c | 25 ++++++
15 files changed, 505 insertions(+)
create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
create mode 100644 tests/qemuxml2argvdata/tseg.args
create mode 100644 tests/qemuxml2argvdata/tseg.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 328f3c0a2386..36f557676fb0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7200,6 +7200,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
return ret;
}
+
+static void
+qemuBuildTSEGCommandLine(virCommandPtr cmd,
+ const virDomainDef *def)
+{
+ if (!def->tseg_size)
+ return;
+
+ virCommandAddArg(cmd, "-global");
+
+ /* PostParse callback guarantees that the size is divisible by 1 MiB */
+ virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu",
+ def->tseg_size >> 20);
+}
+
+
static int
qemuBuildSmpCommandLine(virCommandPtr cmd,
virDomainDefPtr def)
@@ -9921,6 +9937,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0)
goto error;
+ qemuBuildTSEGCommandLine(cmd, def);
+
if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
goto error;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 881d0ea46a75..3ea9e3d47344 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
}
+static int
+qemuDomainSetDefaultTsegSize(virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+ const char *machine = NULL;
+ char *end_ptr = NULL;
+ unsigned int major = 0;
+ unsigned int minor = 0;
+
+ def->tseg_size = 0;
+
+ if (!qemuDomainIsQ35(def))
+ return 0;
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
+ return 0;
+
+ machine = STRSKIP(def->os.machine, "pc-q35-");
+
+ if (!machine)
+ goto error;
+
+ if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
+ goto error;
+
+ if (*end_ptr != '.')
+ goto error;
+
+ machine = end_ptr + 1;
+
+ if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
+ goto error;
+ if (*end_ptr != '\0')
+ goto error;
+
+ /* QEMU started defaulting to 16MiB after 2.9 */
+ if (major > 2 || (major == 2 && minor > 9))
+ def->tseg_size = 16 * 1024 * 1024;
+
+ return 0;
+
+ error:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot parse QEMU machine type version '%s'"),
+ def->os.machine);
+ return -1;
+}
+
+
+static int
+qemuDomainDefTsegPostParse(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps)
+{
+ if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
+ return 0;
+
+ if (!def->tseg_size)
+ return qemuDomainSetDefaultTsegSize(def, qemuCaps);
+
+ if (!qemuDomainIsQ35(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("SMM TSEG is only supported with q35 machine type"));
+ return -1;
+ }
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Setting TSEG size is not supported with this QEMU binary"));
+ return -1;
+ }
+
+ if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("SMM TSEG size must be divisible by 1 MiB"));
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
qemuDomainDefPostParseBasic(virDomainDefPtr def,
virCapsPtr caps,
@@ -3389,6 +3470,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
if (qemuDomainDefCPUPostParse(def) < 0)
goto cleanup;
+ if (qemuDomainDefTsegPostParse(def, qemuCaps) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
virObjectUnref(cfg);
diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.args b/tests/qemuxml2argvdata/tseg-explicit-size.args
new file mode 100644
index 000000000000..d49c81697e43
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-explicit-size.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
+-global mch.extended-tseg-mbytes=48 \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
+-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.xml b/tests/qemuxml2argvdata/tseg-explicit-size.xml
new file mode 100644
index 000000000000..ae3121048495
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'>
+ <tseg>48</tseg>
+ </smm>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/tseg-i440fx.xml b/tests/qemuxml2argvdata/tseg-i440fx.xml
new file mode 100644
index 000000000000..5bd832d50829
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-i440fx.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'>
+ <tseg unit='MiB'>48</tseg>
+ </smm>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/tseg-invalid-size.xml b/tests/qemuxml2argvdata/tseg-invalid-size.xml
new file mode 100644
index 000000000000..3ac8069a81ce
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-invalid-size.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='q35'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'>
+ <tseg unit='KiB'>12345</tseg>
+ </smm>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.args b/tests/qemuxml2argvdata/tseg-old-machine-type.args
new file mode 100644
index 000000000000..ebbdb15e68f2
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-old-machine-type.args
@@ -0,0 +1,27 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-machine pc-q35-2.9,accel=tcg,usb=off,smm=on,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
+-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.xml b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
new file mode 100644
index 000000000000..d1e42586f144
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
@@ -0,0 +1,21 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.9'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'/>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/tseg.args b/tests/qemuxml2argvdata/tseg.args
new file mode 100644
index 000000000000..995957ef1d58
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
+-global mch.extended-tseg-mbytes=16 \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
+-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
diff --git a/tests/qemuxml2argvdata/tseg.xml b/tests/qemuxml2argvdata/tseg.xml
new file mode 100644
index 000000000000..7a31e348b258
--- /dev/null
+++ b/tests/qemuxml2argvdata/tseg.xml
@@ -0,0 +1,21 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'/>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 78454acb1a41..633dfaaee9a4 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2827,6 +2827,54 @@ mymain(void)
DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");
+ DO_TEST("tseg",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST("tseg-explicit-size",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST("tseg-old-machine-type",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST_PARSE_ERROR("tseg-i440fx",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST_PARSE_ERROR("tseg-explicit-size",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI);
+ DO_TEST_PARSE_ERROR("tseg-invalid-size",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+
/* Test disks with format probing enabled for legacy reasons.
* New tests should not go in this section. */
driver.config->allowDiskFormatProbing = true;
diff --git a/tests/qemuxml2xmloutdata/tseg-explicit-size.xml b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml
new file mode 100644
index 000000000000..e1a6e15b610e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml
@@ -0,0 +1,46 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'>
+ <tseg unit='MiB'>48</tseg>
+ </smm>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='sata' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pcie-root'/>
+ <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+ <model name='i82801b11-bridge'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+ </controller>
+ <controller type='pci' index='2' model='pci-bridge'>
+ <model name='pci-bridge'/>
+ <target chassisNr='2'/>
+ <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+ </controller>
+ <controller type='pci' index='3' model='pcie-root-port'>
+ <model name='ioh3420'/>
+ <target chassis='3' port='0x10'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
new file mode 100644
index 000000000000..594c5c025d2e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
@@ -0,0 +1,44 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.9'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'/>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='sata' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pcie-root'/>
+ <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+ <model name='i82801b11-bridge'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+ </controller>
+ <controller type='pci' index='2' model='pci-bridge'>
+ <model name='pci-bridge'/>
+ <target chassisNr='2'/>
+ <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+ </controller>
+ <controller type='pci' index='3' model='pcie-root-port'>
+ <model name='ioh3420'/>
+ <target chassis='3' port='0x10'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/tseg.xml b/tests/qemuxml2xmloutdata/tseg.xml
new file mode 100644
index 000000000000..954ee9d9ee24
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/tseg.xml
@@ -0,0 +1,46 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <smm state='on'>
+ <tseg unit='MiB'>16</tseg>
+ </smm>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='sata' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pcie-root'/>
+ <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+ <model name='i82801b11-bridge'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+ </controller>
+ <controller type='pci' index='2' model='pci-bridge'>
+ <model name='pci-bridge'/>
+ <target chassisNr='2'/>
+ <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+ </controller>
+ <controller type='pci' index='3' model='pcie-root-port'>
+ <model name='ioh3420'/>
+ <target chassis='3' port='0x10'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 7cedc2b999b3..14824679b81c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1181,6 +1181,31 @@ mymain(void)
QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW,
QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW);
+ DO_TEST("tseg",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST("tseg-explicit-size",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+ DO_TEST("tseg-old-machine-type",
+ QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_PCI_BRIDGE,
+ QEMU_CAPS_DEVICE_IOH3420,
+ QEMU_CAPS_ICH9_AHCI,
+ QEMU_CAPS_MACHINE_SMM_OPT,
+ QEMU_CAPS_VIRTIO_SCSI,
+ QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
+
/* Test disks with format probing enabled for legacy reasons.
* New tests should not go in this section. */
driver.config->allowDiskFormatProbing = true;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 21, 2018 at 05:00:53PM +0200, Martin Kletzander wrote:
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/qemu/qemu_command.c | 18 ++++
> src/qemu/qemu_domain.c | 84 +++++++++++++++++++
> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
> .../tseg-old-machine-type.args | 27 ++++++
> .../tseg-old-machine-type.xml | 21 +++++
> tests/qemuxml2argvdata/tseg.args | 28 +++++++
> tests/qemuxml2argvdata/tseg.xml | 21 +++++
> tests/qemuxml2argvtest.c | 48 +++++++++++
> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> .../tseg-old-machine-type.xml | 44 ++++++++++
> tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
> tests/qemuxml2xmltest.c | 25 ++++++
> 15 files changed, 505 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2argvdata/tseg.args
> create mode 100644 tests/qemuxml2argvdata/tseg.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 881d0ea46a75..3ea9e3d47344 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> }
>
>
>+static int
>+qemuDomainSetDefaultTsegSize(virDomainDef *def,
>+ virQEMUCapsPtr qemuCaps)
>+{
>+ const char *machine = NULL;
>+ char *end_ptr = NULL;
>+ unsigned int major = 0;
>+ unsigned int minor = 0;
>+
>+ def->tseg_size = 0;
>+
>+ if (!qemuDomainIsQ35(def))
>+ return 0;
>+
>+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>+ return 0;
>+
>+ machine = STRSKIP(def->os.machine, "pc-q35-");
>+
>+ if (!machine)
>+ goto error;
>+
>+ if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>+ goto error;
>+
>+ if (*end_ptr != '.')
>+ goto error;
>+
>+ machine = end_ptr + 1;
>+
>+ if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>+ goto error;
>+ if (*end_ptr != '\0')
>+ goto error;
>+
>+ /* QEMU started defaulting to 16MiB after 2.9 */
>+ if (major > 2 || (major == 2 && minor > 9))
Please use virParseVersionString
>+ def->tseg_size = 16 * 1024 * 1024;
>+
>+ return 0;
>+
>+ error:
>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>+ _("Cannot parse QEMU machine type version '%s'"),
>+ def->os.machine);
>+ return -1;
>+}
>+
>+
>+static int
>+qemuDomainDefTsegPostParse(virDomainDefPtr def,
>+ virQEMUCapsPtr qemuCaps)
>+{
>+ if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
>+ return 0;
>+
>+ if (!def->tseg_size)
>+ return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>+
>+ if (!qemuDomainIsQ35(def)) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+ _("SMM TSEG is only supported with q35 machine type"));
>+ return -1;
>+ }
>+
>+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+ _("Setting TSEG size is not supported with this QEMU binary"));
>+ return -1;
>+ }
>+
>+ if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
Interesting way of writing the % operator.
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+ _("SMM TSEG size must be divisible by 1 MiB"));
>+ return -1;
>+ }
>+
>+ return 0;
>+}
>+
>+
> static int
> qemuDomainDefPostParseBasic(virDomainDefPtr def,
> virCapsPtr caps,
I'm not sure whether this trial-and-error attribute angers some purists,
but they'll have plenty of time to object until the freeze is over.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 30, 2018 at 08:01:10PM +0200, Ján Tomko wrote:
>On Mon, May 21, 2018 at 05:00:53PM +0200, Martin Kletzander wrote:
>>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>>
>>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>---
>> src/qemu/qemu_command.c | 18 ++++
>> src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>> tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>> .../tseg-old-machine-type.args | 27 ++++++
>> .../tseg-old-machine-type.xml | 21 +++++
>> tests/qemuxml2argvdata/tseg.args | 28 +++++++
>> tests/qemuxml2argvdata/tseg.xml | 21 +++++
>> tests/qemuxml2argvtest.c | 48 +++++++++++
>> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>> .../tseg-old-machine-type.xml | 44 ++++++++++
>> tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>> tests/qemuxml2xmltest.c | 25 ++++++
>> 15 files changed, 505 insertions(+)
>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg.args
>> create mode 100644 tests/qemuxml2argvdata/tseg.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>>
>
>>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>index 881d0ea46a75..3ea9e3d47344 100644
>>--- a/src/qemu/qemu_domain.c
>>+++ b/src/qemu/qemu_domain.c
>>@@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
>> }
>>
>>
>>+static int
>>+qemuDomainSetDefaultTsegSize(virDomainDef *def,
>>+ virQEMUCapsPtr qemuCaps)
>>+{
>>+ const char *machine = NULL;
>>+ char *end_ptr = NULL;
>>+ unsigned int major = 0;
>>+ unsigned int minor = 0;
>>+
>>+ def->tseg_size = 0;
>>+
>>+ if (!qemuDomainIsQ35(def))
>>+ return 0;
>>+
>>+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>>+ return 0;
>>+
>>+ machine = STRSKIP(def->os.machine, "pc-q35-");
>>+
>>+ if (!machine)
>>+ goto error;
>>+
>>+ if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>>+ goto error;
>>+
>>+ if (*end_ptr != '.')
>>+ goto error;
>>+
>>+ machine = end_ptr + 1;
>>+
>>+ if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>>+ goto error;
>>+ if (*end_ptr != '\0')
>>+ goto error;
>>+
>>+ /* QEMU started defaulting to 16MiB after 2.9 */
>>+ if (major > 2 || (major == 2 && minor > 9))
>
>Please use virParseVersionString
>
We have that? Cool. There was no patch related to that function since I
started working on libvirt =D And only few of that that just used it.
>>+ def->tseg_size = 16 * 1024 * 1024;
>>+
>>+ return 0;
>>+
>>+ error:
>>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>>+ _("Cannot parse QEMU machine type version '%s'"),
>>+ def->os.machine);
>>+ return -1;
>>+}
>>+
>>+
>>+static int
>>+qemuDomainDefTsegPostParse(virDomainDefPtr def,
>>+ virQEMUCapsPtr qemuCaps)
>>+{
>>+ if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
>>+ return 0;
>>+
>>+ if (!def->tseg_size)
>>+ return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>>+
>>+ if (!qemuDomainIsQ35(def)) {
>>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>+ _("SMM TSEG is only supported with q35 machine type"));
>>+ return -1;
>>+ }
>>+
>>+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>+ _("Setting TSEG size is not supported with this QEMU binary"));
>>+ return -1;
>>+ }
>>+
>>+ if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>
>Interesting way of writing the % operator.
>
>>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>+ _("SMM TSEG size must be divisible by 1 MiB"));
>>+ return -1;
>>+ }
>>+
>>+ return 0;
>>+}
>>+
>>+
>> static int
>> qemuDomainDefPostParseBasic(virDomainDefPtr def,
>> virCapsPtr caps,
>
>I'm not sure whether this trial-and-error attribute angers some purists,
>but they'll have plenty of time to object until the freeze is over.
>
Dou you have any other better idea? Laszlo properly compared it to picking for
example the right amount of memory. How can you know how much do you need? It
can never be automatically guessed. You can have extra, but each use case will
be hindered a different amount by that. Feel free to suggest a better idea or
Cc some purists ;)
>Reviewed-by: Ján Tomko <jtomko@redhat.com>
>
>Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 30, 2018 at 11:58:54PM +0200, Martin Kletzander wrote:
>On Wed, May 30, 2018 at 08:01:10PM +0200, Ján Tomko wrote:
>>On Mon, May 21, 2018 at 05:00:53PM +0200, Martin Kletzander wrote:
>>>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>>>
>>>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>>---
>>> src/qemu/qemu_command.c | 18 ++++
>>> src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>>> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>>> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>>> tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>>> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>>> .../tseg-old-machine-type.args | 27 ++++++
>>> .../tseg-old-machine-type.xml | 21 +++++
>>> tests/qemuxml2argvdata/tseg.args | 28 +++++++
>>> tests/qemuxml2argvdata/tseg.xml | 21 +++++
>>> tests/qemuxml2argvtest.c | 48 +++++++++++
>>> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>>> .../tseg-old-machine-type.xml | 44 ++++++++++
>>> tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>>> tests/qemuxml2xmltest.c | 25 ++++++
>>> 15 files changed, 505 insertions(+)
>>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>>> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>>> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>>> create mode 100644 tests/qemuxml2argvdata/tseg.args
>>> create mode 100644 tests/qemuxml2argvdata/tseg.xml
>>> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>>> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>>> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>>>
>>
>>>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>>index 881d0ea46a75..3ea9e3d47344 100644
>>>--- a/src/qemu/qemu_domain.c
>>>+++ b/src/qemu/qemu_domain.c
>>>@@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
[...]
>>>+static int
>>>+qemuDomainDefTsegPostParse(virDomainDefPtr def,
>>>+ virQEMUCapsPtr qemuCaps)
>>>+{
>>>+ if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
>>>+ return 0;
>>>+
>>>+ if (!def->tseg_size)
>>>+ return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>>>+
>>>+ if (!qemuDomainIsQ35(def)) {
>>>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>+ _("SMM TSEG is only supported with q35 machine type"));
>>>+ return -1;
>>>+ }
>>>+
>>>+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>>>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>+ _("Setting TSEG size is not supported with this QEMU binary"));
>>>+ return -1;
>>>+ }
>>>+
>>>+ if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>>
>>Interesting way of writing the % operator.
>>
More like very interesting brainfart [1] I had there. I mean I almost open-coded
the VIR_ROUND_UP at first and haven't even thought about anything else. Wow.
Actually, in the resulting assembly (comparing -O2) it both streches across 5
lines that are very weirdly different/similar. What's way faster is:
if (def->tseg_size & ((1<<20) - 1))
which just expands to
xorl %eax, %eax
andl $1048575, %edi
and to make it even more confusing, the previous two approaches share the `andl`
line with this one. Scary.
[1] And that's very weak word for this particular "thing".
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This is way too sparse.
On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> src/qemu/qemu_command.c | 18 ++++
> src/qemu/qemu_domain.c | 84 +++++++++++++++++++
> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
> .../tseg-old-machine-type.args | 27 ++++++
> .../tseg-old-machine-type.xml | 21 +++++
> tests/qemuxml2argvdata/tseg.args | 28 +++++++
> tests/qemuxml2argvdata/tseg.xml | 21 +++++
> tests/qemuxml2argvtest.c | 48 +++++++++++
> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> .../tseg-old-machine-type.xml | 44 ++++++++++
> tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
> tests/qemuxml2xmltest.c | 25 ++++++
> 15 files changed, 505 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2argvdata/tseg.args
> create mode 100644 tests/qemuxml2argvdata/tseg.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 328f3c0a2386..36f557676fb0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7200,6 +7200,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> return ret;
> }
>
> +
> +static void
> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
> + const virDomainDef *def)
> +{
> + if (!def->tseg_size)
Since it's not bool or pointer, prefer the tseg_size == 0
> + return;
> +
> + virCommandAddArg(cmd, "-global");
> +
> + /* PostParse callback guarantees that the size is divisible by 1 MiB */
> + virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu",
> + def->tseg_size >> 20);
> +}
> +
> +
> static int
> qemuBuildSmpCommandLine(virCommandPtr cmd,
> virDomainDefPtr def)
> @@ -9921,6 +9937,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0)
> goto error;
>
> + qemuBuildTSEGCommandLine(cmd, def);
> +
> if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
> goto error;
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 881d0ea46a75..3ea9e3d47344 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> }
>
>
> +static int
> +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +{
> + const char *machine = NULL;
> + char *end_ptr = NULL;
> + unsigned int major = 0;
> + unsigned int minor = 0;
> +
> + def->tseg_size = 0;
Pointless since the only way in here is "if (tseg_size == 0)"
> +
> + if (!qemuDomainIsQ35(def))
> + return 0;
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
Reading this now makes me realized _MBYTES is probably unnecessary, IDC
though since it does follow the QEMU name.
> + return 0;
> +
> + machine = STRSKIP(def->os.machine, "pc-q35-");
> +
> + if (!machine)
> + goto error;
> +
> + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> + goto error;
> +
> + if (*end_ptr != '.')
> + goto error;
> +
> + machine = end_ptr + 1;
> +
> + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> + goto error;
> + if (*end_ptr != '\0')
> + goto error;
> +
> + /* QEMU started defaulting to 16MiB after 2.9 */
> + if (major > 2 || (major == 2 && minor > 9))
> + def->tseg_size = 16 * 1024 * 1024;
So, if QEMU defaults to 16MiB, then why do we need so set this at all?
This seems to me we are setting policy which based on history of many
patches before is a no go. I'd say NAK to this, unless there is some
convincing argument made in the commit message and followup responses to
the series (or you can take Jan's R-By and ignore me - your call.
> +
> + return 0;
> +
> + error:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot parse QEMU machine type version '%s'"),
> + def->os.machine);
> + return -1;
> +}
> +
> +
> +static int
> +qemuDomainDefTsegPostParse(virDomainDefPtr def,
While TSEG is what you're adjusting, this is more a 'features' or in
particular SMM features post parse callback.
> + virQEMUCapsPtr qemuCaps)
> +{
> + if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
> + return 0;
> +
> + if (!def->tseg_size)
Similar... prefer tseg_size == 0
> + return qemuDomainSetDefaultTsegSize(def, qemuCaps);
> +
> + if (!qemuDomainIsQ35(def)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("SMM TSEG is only supported with q35 machine type"));
> + return -1;
> + }
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Setting TSEG size is not supported with this QEMU binary"));
> + return -1;
> + }
> +
> + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("SMM TSEG size must be divisible by 1 MiB"));
> + return -1;
> + }
Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother.
Curious that this differs from qemuDomainMemoryDeviceAlignSize which
claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which
returns 1024 unless it's PPC64 which returns 256MiB alignments.
> +
> + return 0;
> +}
> +
> +
> static int
> qemuDomainDefPostParseBasic(virDomainDefPtr def,
> virCapsPtr caps,
> @@ -3389,6 +3470,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> if (qemuDomainDefCPUPostParse(def) < 0)
> goto cleanup;
>
> + if (qemuDomainDefTsegPostParse(def, qemuCaps) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> virObjectUnref(cfg);
> diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.args b/tests/qemuxml2argvdata/tseg-explicit-size.args
> new file mode 100644
> index 000000000000..d49c81697e43
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.args
> @@ -0,0 +1,28 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
> +-global mch.extended-tseg-mbytes=48 \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot c \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
> diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.xml b/tests/qemuxml2argvdata/tseg-explicit-size.xml
> new file mode 100644
> index 000000000000..ae3121048495
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
> @@ -0,0 +1,23 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'>
> + <tseg>48</tseg>
The only difference here from genericxml2xmlindata/tseg.xml is that this
one doesn't have "unit='MiB'"
> + </smm>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/tseg-i440fx.xml b/tests/qemuxml2argvdata/tseg-i440fx.xml
> new file mode 100644
> index 000000000000..5bd832d50829
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-i440fx.xml
> @@ -0,0 +1,23 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'>
> + <tseg unit='MiB'>48</tseg>
> + </smm>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/tseg-invalid-size.xml b/tests/qemuxml2argvdata/tseg-invalid-size.xml
> new file mode 100644
> index 000000000000..3ac8069a81ce
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-invalid-size.xml
> @@ -0,0 +1,23 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='q35'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'>
> + <tseg unit='KiB'>12345</tseg>
> + </smm>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.args b/tests/qemuxml2argvdata/tseg-old-machine-type.args
> new file mode 100644
> index 000000000000..ebbdb15e68f2
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.args
> @@ -0,0 +1,27 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc-q35-2.9,accel=tcg,usb=off,smm=on,dump-guest-core=off \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot c \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
> diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.xml b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
> new file mode 100644
> index 000000000000..d1e42586f144
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
> @@ -0,0 +1,21 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.9'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/tseg.args b/tests/qemuxml2argvdata/tseg.args
> new file mode 100644
> index 000000000000..995957ef1d58
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg.args
> @@ -0,0 +1,28 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-x86_64 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
> +-global mch.extended-tseg-mbytes=16 \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot c \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
> diff --git a/tests/qemuxml2argvdata/tseg.xml b/tests/qemuxml2argvdata/tseg.xml
> new file mode 100644
> index 000000000000..7a31e348b258
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tseg.xml
and yet another one similr to genericxml2xmlindata/tseg.xml - same name,
different directory
> @@ -0,0 +1,21 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 78454acb1a41..633dfaaee9a4 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2827,6 +2827,54 @@ mymain(void)
>
> DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");
>
> + DO_TEST("tseg",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> + DO_TEST("tseg-explicit-size",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> + DO_TEST("tseg-old-machine-type",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
These all should use DO_TEST_CAPS_LATEST
> + DO_TEST_PARSE_ERROR("tseg-i440fx",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
Failure wrong machine type...
> + DO_TEST_PARSE_ERROR("tseg-explicit-size",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI);
Failure because TSEG_MBYTES not provided.
> + DO_TEST_PARSE_ERROR("tseg-invalid-size",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> +
> /* Test disks with format probing enabled for legacy reasons.
> * New tests should not go in this section. */
> driver.config->allowDiskFormatProbing = true;
> diff --git a/tests/qemuxml2xmloutdata/tseg-explicit-size.xml b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> new file mode 100644
> index 000000000000..e1a6e15b610e
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> @@ -0,0 +1,46 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'>
> + <tseg unit='MiB'>48</tseg>
> + </smm>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <controller type='sata' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
> + </controller>
> + <controller type='pci' index='0' model='pcie-root'/>
> + <controller type='pci' index='1' model='dmi-to-pci-bridge'>
> + <model name='i82801b11-bridge'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model name='pci-bridge'/>
> + <target chassisNr='2'/>
> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
> + </controller>
> + <controller type='pci' index='3' model='pcie-root-port'>
> + <model name='ioh3420'/>
> + <target chassis='3' port='0x10'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> new file mode 100644
> index 000000000000..594c5c025d2e
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> @@ -0,0 +1,44 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.9'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <controller type='sata' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
> + </controller>
> + <controller type='pci' index='0' model='pcie-root'/>
> + <controller type='pci' index='1' model='dmi-to-pci-bridge'>
> + <model name='i82801b11-bridge'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model name='pci-bridge'/>
> + <target chassisNr='2'/>
> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
> + </controller>
> + <controller type='pci' index='3' model='pcie-root-port'>
> + <model name='ioh3420'/>
> + <target chassis='3' port='0x10'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/tseg.xml b/tests/qemuxml2xmloutdata/tseg.xml
> new file mode 100644
> index 000000000000..954ee9d9ee24
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/tseg.xml
> @@ -0,0 +1,46 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <smm state='on'>
> + <tseg unit='MiB'>16</tseg>
> + </smm>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <controller type='sata' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
> + </controller>
> + <controller type='pci' index='0' model='pcie-root'/>
> + <controller type='pci' index='1' model='dmi-to-pci-bridge'>
> + <model name='i82801b11-bridge'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
> + </controller>
> + <controller type='pci' index='2' model='pci-bridge'>
> + <model name='pci-bridge'/>
> + <target chassisNr='2'/>
> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
> + </controller>
> + <controller type='pci' index='3' model='pcie-root-port'>
> + <model name='ioh3420'/>
> + <target chassis='3' port='0x10'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 7cedc2b999b3..14824679b81c 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1181,6 +1181,31 @@ mymain(void)
> QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW,
> QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW);
>
> + DO_TEST("tseg",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> + DO_TEST("tseg-explicit-size",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> + DO_TEST("tseg-old-machine-type",
> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_IOH3420,
> + QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_MACHINE_SMM_OPT,
> + QEMU_CAPS_VIRTIO_SCSI,
> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> +
Never quite understood why xml2xml tests need CAPS...
John
> /* Test disks with format probing enabled for legacy reasons.
> * New tests should not go in this section. */
> driver.config->allowDiskFormatProbing = true;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>
> This is way too sparse.
>
> On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> >
> > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > ---
> > src/qemu/qemu_command.c | 18 ++++
> > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
> > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
> > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
> > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
> > .../tseg-old-machine-type.args | 27 ++++++
> > .../tseg-old-machine-type.xml | 21 +++++
> > tests/qemuxml2argvdata/tseg.args | 28 +++++++
> > tests/qemuxml2argvdata/tseg.xml | 21 +++++
> > tests/qemuxml2argvtest.c | 48 +++++++++++
> > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> > .../tseg-old-machine-type.xml | 44 ++++++++++
> > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
> > tests/qemuxml2xmltest.c | 25 ++++++
> > 15 files changed, 505 insertions(+)
> > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
[...]
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 881d0ea46a75..3ea9e3d47344 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > }
> >
> >
> > +static int
> > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > + const char *machine = NULL;
> > + char *end_ptr = NULL;
> > + unsigned int major = 0;
> > + unsigned int minor = 0;
> > +
> > + def->tseg_size = 0;
>
> Pointless since the only way in here is "if (tseg_size == 0)"
>
> > +
> > + if (!qemuDomainIsQ35(def))
> > + return 0;
> > +
> > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>
> Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> though since it does follow the QEMU name.
>
> > + return 0;
> > +
> > + machine = STRSKIP(def->os.machine, "pc-q35-");
> > +
> > + if (!machine)
> > + goto error;
> > +
> > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > + goto error;
> > +
> > + if (*end_ptr != '.')
> > + goto error;
> > +
> > + machine = end_ptr + 1;
> > +
> > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > + goto error;
> > + if (*end_ptr != '\0')
> > + goto error;
> > +
> > + /* QEMU started defaulting to 16MiB after 2.9 */
> > + if (major > 2 || (major == 2 && minor > 9))
> > + def->tseg_size = 16 * 1024 * 1024;
>
> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>
> This seems to me we are setting policy which based on history of many
> patches before is a no go. I'd say NAK to this, unless there is some
> convincing argument made in the commit message and followup responses to
> the series (or you can take Jan's R-By and ignore me - your call.
I agree with John, this whole function should be removed, we don't have
to set the default in config XML. However we should record that default
value into live/status XML to make sure that it will not be changed
during migration and to let the user know what is the default value.
In order to do that, look at qemuProcessRefreshState() function where we
already gather some information from QEMU after starting domain, the
tseg default value can by updated by calling:
qemuMonitorJSONGetObjectProperty() with
path: /machine/q35/mch
property: extended-tseg-mbytes
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
>On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>>
>> This is way too sparse.
>>
>> On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>> >
>> > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> > ---
>> > src/qemu/qemu_command.c | 18 ++++
>> > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>> > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>> > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>> > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>> > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>> > .../tseg-old-machine-type.args | 27 ++++++
>> > .../tseg-old-machine-type.xml | 21 +++++
>> > tests/qemuxml2argvdata/tseg.args | 28 +++++++
>> > tests/qemuxml2argvdata/tseg.xml | 21 +++++
>> > tests/qemuxml2argvtest.c | 48 +++++++++++
>> > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>> > .../tseg-old-machine-type.xml | 44 ++++++++++
>> > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>> > tests/qemuxml2xmltest.c | 25 ++++++
>> > 15 files changed, 505 insertions(+)
>> > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>> > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>> > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>> > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>> > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>> > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> > create mode 100644 tests/qemuxml2argvdata/tseg.args
>> > create mode 100644 tests/qemuxml2argvdata/tseg.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>
>[...]
>
>> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> > index 881d0ea46a75..3ea9e3d47344 100644
>> > --- a/src/qemu/qemu_domain.c
>> > +++ b/src/qemu/qemu_domain.c
>> > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
>> > }
>> >
>> >
>> > +static int
>> > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
>> > + virQEMUCapsPtr qemuCaps)
>> > +{
>> > + const char *machine = NULL;
>> > + char *end_ptr = NULL;
>> > + unsigned int major = 0;
>> > + unsigned int minor = 0;
>> > +
>> > + def->tseg_size = 0;
>>
>> Pointless since the only way in here is "if (tseg_size == 0)"
>>
>> > +
>> > + if (!qemuDomainIsQ35(def))
>> > + return 0;
>> > +
>> > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>>
>> Reading this now makes me realized _MBYTES is probably unnecessary, IDC
>> though since it does follow the QEMU name.
>>
>> > + return 0;
>> > +
>> > + machine = STRSKIP(def->os.machine, "pc-q35-");
>> > +
>> > + if (!machine)
>> > + goto error;
>> > +
>> > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>> > + goto error;
>> > +
>> > + if (*end_ptr != '.')
>> > + goto error;
>> > +
>> > + machine = end_ptr + 1;
>> > +
>> > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>> > + goto error;
>> > + if (*end_ptr != '\0')
>> > + goto error;
>> > +
>> > + /* QEMU started defaulting to 16MiB after 2.9 */
>> > + if (major > 2 || (major == 2 && minor > 9))
>> > + def->tseg_size = 16 * 1024 * 1024;
>>
>> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>>
>> This seems to me we are setting policy which based on history of many
>> patches before is a no go. I'd say NAK to this, unless there is some
>> convincing argument made in the commit message and followup responses to
>> the series (or you can take Jan's R-By and ignore me - your call.
>
>I agree with John, this whole function should be removed, we don't have
>to set the default in config XML. However we should record that default
>value into live/status XML to make sure that it will not be changed
>during migration and to let the user know what is the default value.
>
That doesn't make sense. This is part of guest ABI and if you want to be on the
safe side when migrating, then you should be way more cautious with the
stop/start scenario. Migration will probably fail (or silently corrupt data,
but I believe that wouldn't happen), but stop/start without keeping the default
will change that in case QEMU changes default and then the guest ABI will
change. See my other reply to this particular concern.
>In order to do that, look at qemuProcessRefreshState() function where we
>already gather some information from QEMU after starting domain, the
>tseg default value can by updated by calling:
>
> qemuMonitorJSONGetObjectProperty() with
> path: /machine/q35/mch
> property: extended-tseg-mbytes
>
>Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
> On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> > >
> > > This is way too sparse.
> > >
> > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > > >
> > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > > > ---
> > > > src/qemu/qemu_command.c | 18 ++++
> > > > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
> > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
> > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> > > > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
> > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
> > > > .../tseg-old-machine-type.args | 27 ++++++
> > > > .../tseg-old-machine-type.xml | 21 +++++
> > > > tests/qemuxml2argvdata/tseg.args | 28 +++++++
> > > > tests/qemuxml2argvdata/tseg.xml | 21 +++++
> > > > tests/qemuxml2argvtest.c | 48 +++++++++++
> > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> > > > .../tseg-old-machine-type.xml | 44 ++++++++++
> > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
> > > > tests/qemuxml2xmltest.c | 25 ++++++
> > > > 15 files changed, 505 insertions(+)
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
> >
> > [...]
> >
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 881d0ea46a75..3ea9e3d47344 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > > > }
> > > >
> > > >
> > > > +static int
> > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > > + virQEMUCapsPtr qemuCaps)
> > > > +{
> > > > + const char *machine = NULL;
> > > > + char *end_ptr = NULL;
> > > > + unsigned int major = 0;
> > > > + unsigned int minor = 0;
> > > > +
> > > > + def->tseg_size = 0;
> > >
> > > Pointless since the only way in here is "if (tseg_size == 0)"
> > >
> > > > +
> > > > + if (!qemuDomainIsQ35(def))
> > > > + return 0;
> > > > +
> > > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > >
> > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> > > though since it does follow the QEMU name.
> > >
> > > > + return 0;
> > > > +
> > > > + machine = STRSKIP(def->os.machine, "pc-q35-");
> > > > +
> > > > + if (!machine)
> > > > + goto error;
> > > > +
> > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > > + goto error;
> > > > +
> > > > + if (*end_ptr != '.')
> > > > + goto error;
> > > > +
> > > > + machine = end_ptr + 1;
> > > > +
> > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > > + goto error;
> > > > + if (*end_ptr != '\0')
> > > > + goto error;
> > > > +
> > > > + /* QEMU started defaulting to 16MiB after 2.9 */
> > > > + if (major > 2 || (major == 2 && minor > 9))
> > > > + def->tseg_size = 16 * 1024 * 1024;
> > >
> > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> > >
> > > This seems to me we are setting policy which based on history of many
> > > patches before is a no go. I'd say NAK to this, unless there is some
> > > convincing argument made in the commit message and followup responses to
> > > the series (or you can take Jan's R-By and ignore me - your call.
> >
> > I agree with John, this whole function should be removed, we don't have
> > to set the default in config XML. However we should record that default
> > value into live/status XML to make sure that it will not be changed
> > during migration and to let the user know what is the default value.
> >
>
> That doesn't make sense. This is part of guest ABI and if you want to be on the
> safe side when migrating, then you should be way more cautious with the
> stop/start scenario. Migration will probably fail (or silently corrupt data,
> but I believe that wouldn't happen), but stop/start without keeping the default
> will change that in case QEMU changes default and then the guest ABI will
> change. See my other reply to this particular concern.
I'm not sure that's true. You can consider CPU as guest ABI but using
host-model the actual CPU is described only in live/status XML but the
config XML always has mode set to host-model.
Another thing is that if user doesn't care about this value they would
lose the benefit of using the default hypervisor value and once it get's
updated in the hypervisor they would be stuck with the previous default.
We might need to somehow specify what guest ABI means and what it should
cover and what can be left to hypervisor.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
>On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
>> On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
>> > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>> > >
>> > > This is way too sparse.
>> > >
>> > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>> > > >
>> > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> > > > ---
>> > > > src/qemu/qemu_command.c | 18 ++++
>> > > > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>> > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>> > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>> > > > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>> > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>> > > > .../tseg-old-machine-type.args | 27 ++++++
>> > > > .../tseg-old-machine-type.xml | 21 +++++
>> > > > tests/qemuxml2argvdata/tseg.args | 28 +++++++
>> > > > tests/qemuxml2argvdata/tseg.xml | 21 +++++
>> > > > tests/qemuxml2argvtest.c | 48 +++++++++++
>> > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>> > > > .../tseg-old-machine-type.xml | 44 ++++++++++
>> > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>> > > > tests/qemuxml2xmltest.c | 25 ++++++
>> > > > 15 files changed, 505 insertions(+)
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
>> > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
>> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>> >
>> > [...]
>> >
>> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> > > > index 881d0ea46a75..3ea9e3d47344 100644
>> > > > --- a/src/qemu/qemu_domain.c
>> > > > +++ b/src/qemu/qemu_domain.c
>> > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
>> > > > }
>> > > >
>> > > >
>> > > > +static int
>> > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
>> > > > + virQEMUCapsPtr qemuCaps)
>> > > > +{
>> > > > + const char *machine = NULL;
>> > > > + char *end_ptr = NULL;
>> > > > + unsigned int major = 0;
>> > > > + unsigned int minor = 0;
>> > > > +
>> > > > + def->tseg_size = 0;
>> > >
>> > > Pointless since the only way in here is "if (tseg_size == 0)"
>> > >
>> > > > +
>> > > > + if (!qemuDomainIsQ35(def))
>> > > > + return 0;
>> > > > +
>> > > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>> > >
>> > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
>> > > though since it does follow the QEMU name.
>> > >
>> > > > + return 0;
>> > > > +
>> > > > + machine = STRSKIP(def->os.machine, "pc-q35-");
>> > > > +
>> > > > + if (!machine)
>> > > > + goto error;
>> > > > +
>> > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>> > > > + goto error;
>> > > > +
>> > > > + if (*end_ptr != '.')
>> > > > + goto error;
>> > > > +
>> > > > + machine = end_ptr + 1;
>> > > > +
>> > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>> > > > + goto error;
>> > > > + if (*end_ptr != '\0')
>> > > > + goto error;
>> > > > +
>> > > > + /* QEMU started defaulting to 16MiB after 2.9 */
>> > > > + if (major > 2 || (major == 2 && minor > 9))
>> > > > + def->tseg_size = 16 * 1024 * 1024;
>> > >
>> > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>> > >
>> > > This seems to me we are setting policy which based on history of many
>> > > patches before is a no go. I'd say NAK to this, unless there is some
>> > > convincing argument made in the commit message and followup responses to
>> > > the series (or you can take Jan's R-By and ignore me - your call.
>> >
>> > I agree with John, this whole function should be removed, we don't have
>> > to set the default in config XML. However we should record that default
>> > value into live/status XML to make sure that it will not be changed
>> > during migration and to let the user know what is the default value.
>> >
>>
>> That doesn't make sense. This is part of guest ABI and if you want to be on the
>> safe side when migrating, then you should be way more cautious with the
>> stop/start scenario. Migration will probably fail (or silently corrupt data,
>> but I believe that wouldn't happen), but stop/start without keeping the default
>> will change that in case QEMU changes default and then the guest ABI will
>> change. See my other reply to this particular concern.
>
>I'm not sure that's true. You can consider CPU as guest ABI but using
>host-model the actual CPU is described only in live/status XML but the
>config XML always has mode set to host-model.
>
Well, but you used the wrong example. The documentation for host-model properly
says:
"...but shutting down and restarting the guest may present different hardware
to the guest according to the capabilities of the new host."
host-model is one of the special cases where we cannot provide stable guest ABI
and we explicitly say that.
>Another thing is that if user doesn't care about this value they would
>lose the benefit of using the default hypervisor value and once it get's
>updated in the hypervisor they would be stuck with the previous default.
>
Yes. But that's actually precisely the reason we are doing this.
>We might need to somehow specify what guest ABI means and what it should
>cover and what can be left to hypervisor.
>
Well, we don't, guest ABI is not a term we would thought of. Whatever the guest
OS sees as part of the hardware, whatever it can gather about the virtual
machine is part of the guest ABI.
Don't think of this as host-model, but rather chassisNr. For very brief, but
good, explanation see commit 8dc88aeed612aeea1ae0d249ec760938cecce5aa.
>Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote:
> On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
> > On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
> > > On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> > > > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> > > > >
> > > > > This is way too sparse.
> > > > >
> > > > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > > > > >
> > > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > > > > > ---
> > > > > > src/qemu/qemu_command.c | 18 ++++
> > > > > > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
> > > > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
> > > > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> > > > > > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
> > > > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
> > > > > > .../tseg-old-machine-type.args | 27 ++++++
> > > > > > .../tseg-old-machine-type.xml | 21 +++++
> > > > > > tests/qemuxml2argvdata/tseg.args | 28 +++++++
> > > > > > tests/qemuxml2argvdata/tseg.xml | 21 +++++
> > > > > > tests/qemuxml2argvtest.c | 48 +++++++++++
> > > > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> > > > > > .../tseg-old-machine-type.xml | 44 ++++++++++
> > > > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
> > > > > > tests/qemuxml2xmltest.c | 25 ++++++
> > > > > > 15 files changed, 505 insertions(+)
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
> > > >
> > > > [...]
> > > >
> > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > > > index 881d0ea46a75..3ea9e3d47344 100644
> > > > > > --- a/src/qemu/qemu_domain.c
> > > > > > +++ b/src/qemu/qemu_domain.c
> > > > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > > > > > }
> > > > > >
> > > > > >
> > > > > > +static int
> > > > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > > > > + virQEMUCapsPtr qemuCaps)
> > > > > > +{
> > > > > > + const char *machine = NULL;
> > > > > > + char *end_ptr = NULL;
> > > > > > + unsigned int major = 0;
> > > > > > + unsigned int minor = 0;
> > > > > > +
> > > > > > + def->tseg_size = 0;
> > > > >
> > > > > Pointless since the only way in here is "if (tseg_size == 0)"
> > > > >
> > > > > > +
> > > > > > + if (!qemuDomainIsQ35(def))
> > > > > > + return 0;
> > > > > > +
> > > > > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > > > >
> > > > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> > > > > though since it does follow the QEMU name.
> > > > >
> > > > > > + return 0;
> > > > > > +
> > > > > > + machine = STRSKIP(def->os.machine, "pc-q35-");
> > > > > > +
> > > > > > + if (!machine)
> > > > > > + goto error;
> > > > > > +
> > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > > > > + goto error;
> > > > > > +
> > > > > > + if (*end_ptr != '.')
> > > > > > + goto error;
> > > > > > +
> > > > > > + machine = end_ptr + 1;
> > > > > > +
> > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > > > > + goto error;
> > > > > > + if (*end_ptr != '\0')
> > > > > > + goto error;
> > > > > > +
> > > > > > + /* QEMU started defaulting to 16MiB after 2.9 */
> > > > > > + if (major > 2 || (major == 2 && minor > 9))
> > > > > > + def->tseg_size = 16 * 1024 * 1024;
> > > > >
> > > > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> > > > >
> > > > > This seems to me we are setting policy which based on history of many
> > > > > patches before is a no go. I'd say NAK to this, unless there is some
> > > > > convincing argument made in the commit message and followup responses to
> > > > > the series (or you can take Jan's R-By and ignore me - your call.
> > > >
> > > > I agree with John, this whole function should be removed, we don't have
> > > > to set the default in config XML. However we should record that default
> > > > value into live/status XML to make sure that it will not be changed
> > > > during migration and to let the user know what is the default value.
> > > >
> > >
> > > That doesn't make sense. This is part of guest ABI and if you want to be on the
> > > safe side when migrating, then you should be way more cautious with the
> > > stop/start scenario. Migration will probably fail (or silently corrupt data,
> > > but I believe that wouldn't happen), but stop/start without keeping the default
> > > will change that in case QEMU changes default and then the guest ABI will
> > > change. See my other reply to this particular concern.
> >
> > I'm not sure that's true. You can consider CPU as guest ABI but using
> > host-model the actual CPU is described only in live/status XML but the
> > config XML always has mode set to host-model.
> >
>
> Well, but you used the wrong example. The documentation for host-model properly
> says:
>
> "...but shutting down and restarting the guest may present different hardware
> to the guest according to the capabilities of the new host."
>
> host-model is one of the special cases where we cannot provide stable guest ABI
> and we explicitly say that.
>
> > Another thing is that if user doesn't care about this value they would
> > lose the benefit of using the default hypervisor value and once it get's
> > updated in the hypervisor they would be stuck with the previous default.
> >
>
> Yes. But that's actually precisely the reason we are doing this.
>
> > We might need to somehow specify what guest ABI means and what it should
> > cover and what can be left to hypervisor.
> >
>
> Well, we don't, guest ABI is not a term we would thought of. Whatever the guest
> OS sees as part of the hardware, whatever it can gather about the virtual
> machine is part of the guest ABI.
Agreed, I should have express myself precisely, it's definitely guest
ABI. What I meant is that some changes are not that critical to the
guest OS or they don't have any real effect.
In this case it's just a size of memory, it's not like it will
disable/enable that feature. FWIU the size can affect whether some
guest with lot of vcpus and/or with lot of memory will not boot.
So yes, ideally we should record the default value also into the config
XML to make sure that once the guest is installed/configured the value
is not changed. The only thing that I don't personally like is that
the value is hard-coded into libvirt and in the future we will most
likely change it.
One possible compromise could be that we would not set any value at
define time, instead if no value is configured we would get the default
from QEMU and propagate that default into config as well to make sure
that every other time the guest is started again it will have the same
ABI. I hope that this idea is not too crazy :).
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 03:52:26PM +0200, Pavel Hrdina wrote:
>On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote:
>> On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
>> > On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
>> > > On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
>> > > > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>> > > > >
>> > > > > This is way too sparse.
>> > > > >
>> > > > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>> > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>> > > > > >
>> > > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> > > > > > ---
>> > > > > > src/qemu/qemu_command.c | 18 ++++
>> > > > > > src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>> > > > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>> > > > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>> > > > > > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>> > > > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>> > > > > > .../tseg-old-machine-type.args | 27 ++++++
>> > > > > > .../tseg-old-machine-type.xml | 21 +++++
>> > > > > > tests/qemuxml2argvdata/tseg.args | 28 +++++++
>> > > > > > tests/qemuxml2argvdata/tseg.xml | 21 +++++
>> > > > > > tests/qemuxml2argvtest.c | 48 +++++++++++
>> > > > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>> > > > > > .../tseg-old-machine-type.xml | 44 ++++++++++
>> > > > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>> > > > > > tests/qemuxml2xmltest.c | 25 ++++++
>> > > > > > 15 files changed, 505 insertions(+)
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
>> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
>> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>> > > >
>> > > > [...]
>> > > >
>> > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> > > > > > index 881d0ea46a75..3ea9e3d47344 100644
>> > > > > > --- a/src/qemu/qemu_domain.c
>> > > > > > +++ b/src/qemu/qemu_domain.c
>> > > > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
>> > > > > > }
>> > > > > >
>> > > > > >
>> > > > > > +static int
>> > > > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
>> > > > > > + virQEMUCapsPtr qemuCaps)
>> > > > > > +{
>> > > > > > + const char *machine = NULL;
>> > > > > > + char *end_ptr = NULL;
>> > > > > > + unsigned int major = 0;
>> > > > > > + unsigned int minor = 0;
>> > > > > > +
>> > > > > > + def->tseg_size = 0;
>> > > > >
>> > > > > Pointless since the only way in here is "if (tseg_size == 0)"
>> > > > >
>> > > > > > +
>> > > > > > + if (!qemuDomainIsQ35(def))
>> > > > > > + return 0;
>> > > > > > +
>> > > > > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>> > > > >
>> > > > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
>> > > > > though since it does follow the QEMU name.
>> > > > >
>> > > > > > + return 0;
>> > > > > > +
>> > > > > > + machine = STRSKIP(def->os.machine, "pc-q35-");
>> > > > > > +
>> > > > > > + if (!machine)
>> > > > > > + goto error;
>> > > > > > +
>> > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>> > > > > > + goto error;
>> > > > > > +
>> > > > > > + if (*end_ptr != '.')
>> > > > > > + goto error;
>> > > > > > +
>> > > > > > + machine = end_ptr + 1;
>> > > > > > +
>> > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>> > > > > > + goto error;
>> > > > > > + if (*end_ptr != '\0')
>> > > > > > + goto error;
>> > > > > > +
>> > > > > > + /* QEMU started defaulting to 16MiB after 2.9 */
>> > > > > > + if (major > 2 || (major == 2 && minor > 9))
>> > > > > > + def->tseg_size = 16 * 1024 * 1024;
>> > > > >
>> > > > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>> > > > >
>> > > > > This seems to me we are setting policy which based on history of many
>> > > > > patches before is a no go. I'd say NAK to this, unless there is some
>> > > > > convincing argument made in the commit message and followup responses to
>> > > > > the series (or you can take Jan's R-By and ignore me - your call.
>> > > >
>> > > > I agree with John, this whole function should be removed, we don't have
>> > > > to set the default in config XML. However we should record that default
>> > > > value into live/status XML to make sure that it will not be changed
>> > > > during migration and to let the user know what is the default value.
>> > > >
>> > >
>> > > That doesn't make sense. This is part of guest ABI and if you want to be on the
>> > > safe side when migrating, then you should be way more cautious with the
>> > > stop/start scenario. Migration will probably fail (or silently corrupt data,
>> > > but I believe that wouldn't happen), but stop/start without keeping the default
>> > > will change that in case QEMU changes default and then the guest ABI will
>> > > change. See my other reply to this particular concern.
>> >
>> > I'm not sure that's true. You can consider CPU as guest ABI but using
>> > host-model the actual CPU is described only in live/status XML but the
>> > config XML always has mode set to host-model.
>> >
>>
>> Well, but you used the wrong example. The documentation for host-model properly
>> says:
>>
>> "...but shutting down and restarting the guest may present different hardware
>> to the guest according to the capabilities of the new host."
>>
>> host-model is one of the special cases where we cannot provide stable guest ABI
>> and we explicitly say that.
>>
>> > Another thing is that if user doesn't care about this value they would
>> > lose the benefit of using the default hypervisor value and once it get's
>> > updated in the hypervisor they would be stuck with the previous default.
>> >
>>
>> Yes. But that's actually precisely the reason we are doing this.
>>
>> > We might need to somehow specify what guest ABI means and what it should
>> > cover and what can be left to hypervisor.
>> >
>>
>> Well, we don't, guest ABI is not a term we would thought of. Whatever the guest
>> OS sees as part of the hardware, whatever it can gather about the virtual
>> machine is part of the guest ABI.
>
>Agreed, I should have express myself precisely, it's definitely guest
>ABI. What I meant is that some changes are not that critical to the
>guest OS or they don't have any real effect.
>
>In this case it's just a size of memory, it's not like it will
>disable/enable that feature. FWIU the size can affect whether some
>guest with lot of vcpus and/or with lot of memory will not boot.
>
>So yes, ideally we should record the default value also into the config
>XML to make sure that once the guest is installed/configured the value
>is not changed. The only thing that I don't personally like is that
>the value is hard-coded into libvirt and in the future we will most
>likely change it.
>
>One possible compromise could be that we would not set any value at
>define time, instead if no value is configured we would get the default
>from QEMU and propagate that default into config as well to make sure
>that every other time the guest is started again it will have the same
>ABI. I hope that this idea is not too crazy :).
>
No, that's one of the possibilities. We might also believe QEMU that
they will not change the default without machine type change and be done
with it. It will have the same effect as if the VM was launched before
with older libvirt and then with an updated one, which got updated with
QEMU as well. If QEMU doesn't change the default per machine type, then
we're fine. It seems like an okay way to go. At least I can yell "told
you so" later in case QEMU changes that default and someone asks us to
start recording the 16MiB value and it's going to be too late =) But
that's not going to happen, right?
I'll send a simplified v2 and we'll see how that goes.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>
>This is way too sparse.
>
I can't really think of what to say here that is not already mentioned in the
documentation or self-explanatory in the code. But maybe only I see that as
self-explanatory. I'll write something up here.
>On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> src/qemu/qemu_command.c | 18 ++++
>> src/qemu/qemu_domain.c | 84 +++++++++++++++++++
>> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++
>> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
>> tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++
>> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++
>> .../tseg-old-machine-type.args | 27 ++++++
>> .../tseg-old-machine-type.xml | 21 +++++
>> tests/qemuxml2argvdata/tseg.args | 28 +++++++
>> tests/qemuxml2argvdata/tseg.xml | 21 +++++
>> tests/qemuxml2argvtest.c | 48 +++++++++++
>> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
>> .../tseg-old-machine-type.xml | 44 ++++++++++
>> tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++
>> tests/qemuxml2xmltest.c | 25 ++++++
>> 15 files changed, 505 insertions(+)
>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
>> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
>> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> create mode 100644 tests/qemuxml2argvdata/tseg.args
>> create mode 100644 tests/qemuxml2argvdata/tseg.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 328f3c0a2386..36f557676fb0 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7200,6 +7200,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>> return ret;
>> }
>>
>> +
>> +static void
>> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
>> + const virDomainDef *def)
>> +{
>> + if (!def->tseg_size)
>
>Since it's not bool or pointer, prefer the tseg_size == 0
>
I don't know if you mean that as indicative or imperative. It is very
subjective and for such scenarios I would like to execute my right to mention
that it is not part of Contributor Guidelines. I know it might seem rude, but
this is one of the things that's very subjective and for such things I like to
first reach a consensus before I change what I'm used to writing. I hope you
understand that.
>> + return;
>> +
>> + virCommandAddArg(cmd, "-global");
>> +
>> + /* PostParse callback guarantees that the size is divisible by 1 MiB */
>> + virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu",
>> + def->tseg_size >> 20);
>> +}
>> +
>> +
>> static int
>> qemuBuildSmpCommandLine(virCommandPtr cmd,
>> virDomainDefPtr def)
>> @@ -9921,6 +9937,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>> if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0)
>> goto error;
>>
>> + qemuBuildTSEGCommandLine(cmd, def);
>> +
>> if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
>> goto error;
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 881d0ea46a75..3ea9e3d47344 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
>> }
>>
>>
>> +static int
>> +qemuDomainSetDefaultTsegSize(virDomainDef *def,
>> + virQEMUCapsPtr qemuCaps)
>> +{
>> + const char *machine = NULL;
>> + char *end_ptr = NULL;
>> + unsigned int major = 0;
>> + unsigned int minor = 0;
>> +
>> + def->tseg_size = 0;
>
>Pointless since the only way in here is "if (tseg_size == 0)"
>
My bad for not commenting the function. But with this line the function
is self-sufficient and clearly does what the name suggests (set the
default TSEG size) no matter where it is called from and under what
circumstances/conditions.
>> +
>> + if (!qemuDomainIsQ35(def))
>> + return 0;
>> +
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
>
>Reading this now makes me realized _MBYTES is probably unnecessary, IDC
>though since it does follow the QEMU name.
>
That's exactly why I chose that naming (after changing it many, _many_
times). I imagined there will come a time when QEMU adds an option
named mch.extended_tseg(_maybe_some_suffix) and the person adding a
capability for that will not adjust the naming on this one. I see this
as:
a) rather safe than sorry
b) very much precise with what it describes
>> + return 0;
>> +
>> + machine = STRSKIP(def->os.machine, "pc-q35-");
>> +
>> + if (!machine)
>> + goto error;
>> +
>> + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
>> + goto error;
>> +
>> + if (*end_ptr != '.')
>> + goto error;
>> +
>> + machine = end_ptr + 1;
>> +
>> + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
>> + goto error;
>> + if (*end_ptr != '\0')
>> + goto error;
>> +
>> + /* QEMU started defaulting to 16MiB after 2.9 */
>> + if (major > 2 || (major == 2 && minor > 9))
>> + def->tseg_size = 16 * 1024 * 1024;
>
>So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>
TL;DR because not setting the default value when it is not explicitly
requested has already bitten us in the back multiple times.
Long story short this way we are safe. No matter what happens (QEMU
changing the default, the user changing the config somewhere else and
not expecting this to change, us wanting to change the default in the
future for some reason, migration to newer libvirt where who-knows-what
is checked there, etc.) it is way easier to keep the guest ABI stable.
It is visible what the value actually is and we're not hiding it
somewhere in the code of the hypervisor. I know we don't do that for
many things. I could've gone with the (often alibistic [1]) "the
default is left for hypervisor to decide", but since we have the
opportunity tomake things right (thanks to Laszlo's explanations), why
shouldn't we?
>This seems to me we are setting policy which based on history of many
>patches before is a no go. I'd say NAK to this, unless there is some
>convincing argument made in the commit message and followup responses to
>the series (or you can take Jan's R-By and ignore me - your call.
>
What patches are you referring to? I can add that to the commit
message, sure.
>> +
>> + return 0;
>> +
>> + error:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Cannot parse QEMU machine type version '%s'"),
>> + def->os.machine);
>> + return -1;
>> +}
>> +
>> +
>> +static int
>> +qemuDomainDefTsegPostParse(virDomainDefPtr def,
>
>While TSEG is what you're adjusting, this is more a 'features' or in
>particular SMM features post parse callback.
>
I'm getting the hint of a hidden suggestion from the sentence. Are you
suggesting I should rename the function or create few more layers like
this?
qemuDomainDefSMMFeaturesPostParse() {
qemuDomainDefTsegPostParse();
}
qemuDomainDefFeaturesPostParse() {
qemuDomainDefSMMFeaturesPostParse();
}
qemuDomainDefPostParse() {
...
qemuDomainDefFeaturesPostParse();
...
}
>> + virQEMUCapsPtr qemuCaps)
>> +{
>> + if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
>> + return 0;
>> +
>> + if (!def->tseg_size)
>
>Similar... prefer tseg_size == 0
>
Again, are you directing me to do that or saying what version you like more?
>> + return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>> +
>> + if (!qemuDomainIsQ35(def)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("SMM TSEG is only supported with q35 machine type"));
>> + return -1;
>> + }
>> +
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Setting TSEG size is not supported with this QEMU binary"));
>> + return -1;
>> + }
>> +
>> + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("SMM TSEG size must be divisible by 1 MiB"));
>> + return -1;
>> + }
>
>Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother.
>
>Curious that this differs from qemuDomainMemoryDeviceAlignSize which
>claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which
>returns 1024 unless it's PPC64 which returns 256MiB alignments.
It does. The extended TSEG explicitly allows 1 MiB increments. Not
only because of the naming (*_mbytes), but also because bigger
granularity would just be pointless probably. Also this is not going to
exist on PPC64, it only makes sense with q35.
>> +
>> + return 0;
>> +}
>> +
>> +
>> static int
>> qemuDomainDefPostParseBasic(virDomainDefPtr def,
>> virCapsPtr caps,
>> @@ -3389,6 +3470,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>> if (qemuDomainDefCPUPostParse(def) < 0)
>> goto cleanup;
>>
>> + if (qemuDomainDefTsegPostParse(def, qemuCaps) < 0)
>> + goto cleanup;
>> +
>> ret = 0;
>> cleanup:
>> virObjectUnref(cfg);
>> diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.args b/tests/qemuxml2argvdata/tseg-explicit-size.args
>> new file mode 100644
>> index 000000000000..d49c81697e43
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.args
>> @@ -0,0 +1,28 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu-system-x86_64 \
>> +-name QEMUGuest1 \
>> +-S \
>> +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
>> +-global mch.extended-tseg-mbytes=48 \
>> +-m 214 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-display none \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
>> +server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-no-acpi \
>> +-boot c \
>> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
>> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
>> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
>> diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.xml b/tests/qemuxml2argvdata/tseg-explicit-size.xml
>> new file mode 100644
>> index 000000000000..ae3121048495
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
>> @@ -0,0 +1,23 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'>
>> + <tseg>48</tseg>
>
>The only difference here from genericxml2xmlindata/tseg.xml is that this
>one doesn't have "unit='MiB'"
>
Yes, I wanted to check for the correct parsing of both. Maybe I should
put 1 GiB in the other one so that I check that it parses the unit
properly? Actually I have KiB somewhere, so probably not.
>> + </smm>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvdata/tseg-i440fx.xml b/tests/qemuxml2argvdata/tseg-i440fx.xml
>> new file mode 100644
>> index 000000000000..5bd832d50829
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-i440fx.xml
>> @@ -0,0 +1,23 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'>
>> + <tseg unit='MiB'>48</tseg>
>> + </smm>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvdata/tseg-invalid-size.xml b/tests/qemuxml2argvdata/tseg-invalid-size.xml
>> new file mode 100644
>> index 000000000000..3ac8069a81ce
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-invalid-size.xml
>> @@ -0,0 +1,23 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='q35'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'>
>> + <tseg unit='KiB'>12345</tseg>
>> + </smm>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.args b/tests/qemuxml2argvdata/tseg-old-machine-type.args
>> new file mode 100644
>> index 000000000000..ebbdb15e68f2
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.args
>> @@ -0,0 +1,27 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu-system-x86_64 \
>> +-name QEMUGuest1 \
>> +-S \
>> +-machine pc-q35-2.9,accel=tcg,usb=off,smm=on,dump-guest-core=off \
>> +-m 214 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-display none \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
>> +server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-no-acpi \
>> +-boot c \
>> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
>> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
>> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
>> diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.xml b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> new file mode 100644
>> index 000000000000..d1e42586f144
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.xml
>> @@ -0,0 +1,21 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc-q35-2.9'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'/>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvdata/tseg.args b/tests/qemuxml2argvdata/tseg.args
>> new file mode 100644
>> index 000000000000..995957ef1d58
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg.args
>> @@ -0,0 +1,28 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu-system-x86_64 \
>> +-name QEMUGuest1 \
>> +-S \
>> +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \
>> +-global mch.extended-tseg-mbytes=16 \
>> +-m 214 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-display none \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
>> +server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-no-acpi \
>> +-boot c \
>> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
>> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
>> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
>> diff --git a/tests/qemuxml2argvdata/tseg.xml b/tests/qemuxml2argvdata/tseg.xml
>> new file mode 100644
>> index 000000000000..7a31e348b258
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg.xml
>
>and yet another one similr to genericxml2xmlindata/tseg.xml - same name,
>different directory
>
Are you suggesting I create this as a symlink? Or that I should add
info to the commit message that this patch adds checks for formatting
the command-line (the functionality the patch is adding) and that the
genericxml2xmltest is checking the parsing even for developers who build
with QEMU driver disabled?
>> @@ -0,0 +1,21 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'/>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 78454acb1a41..633dfaaee9a4 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2827,6 +2827,54 @@ mymain(void)
>>
>> DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");
>>
>> + DO_TEST("tseg",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> + DO_TEST("tseg-explicit-size",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> + DO_TEST("tseg-old-machine-type",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>
>These all should use DO_TEST_CAPS_LATEST
>
OK, good point.
>> + DO_TEST_PARSE_ERROR("tseg-i440fx",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>Failure wrong machine type...
>
>> + DO_TEST_PARSE_ERROR("tseg-explicit-size",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI);
>
>Failure because TSEG_MBYTES not provided.
>
I'm sorry, don't take this personally, but sometimes I'm not sure
whether you are trying to convey some information or you are just adding
comments for yourself. If you are asking me to do something, then
please say so. If you want these messages to be there as comments, let
me know, I'll do that (although the only time that would be useful is if
someone makes a change and the tests start failing (by the tested code
not failing) and that person wants to know what was supposed to be the
error. In which case they can re-run them without the modifications.
Or if you are suggesting that it should be DO_TEST_FAILURE instead of
DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the
code doesn't throw an error during starting the domain, but right away
during parsing. And yes, that can be done because I'm not erroring out
for something that existed before, but for a new feature so it will
never be read from the disk for a domain that existed before this
series. Or maybe I ran the tests wrong and it is supposed to be
DO_TEST_FAILURE and I made a mistake somewhere else as well.
The reason for me not really being sure what you mean by that is that I
take all of what's in the previous paragraph as something obvious and
known by default for non-newbie libvirt developers (which you most
certainly are one since you have years of experience in this codebase),
so I'm probably just missing something. It very well might be a
language barrier, because I'm nowhere near understanding most of the
nuances in English language and various dialects thereof. So with no
hidden meaning between the lines, please enlighten me.
>> + DO_TEST_PARSE_ERROR("tseg-invalid-size",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> +
>> /* Test disks with format probing enabled for legacy reasons.
>> * New tests should not go in this section. */
>> driver.config->allowDiskFormatProbing = true;
>> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
>> index 7cedc2b999b3..14824679b81c 100644
>> --- a/tests/qemuxml2xmltest.c
>> +++ b/tests/qemuxml2xmltest.c
>> @@ -1181,6 +1181,31 @@ mymain(void)
>> QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW,
>> QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW);
>>
>> + DO_TEST("tseg",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> + DO_TEST("tseg-explicit-size",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> + DO_TEST("tseg-old-machine-type",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> +
>
>Never quite understood why xml2xml tests need CAPS...
>
Well, I could rant about that for a while. Basically it is because we
want to do some modifications during parsing, so not only tests, but
also the rest of the code has capabilities already available when
parsing the domein. If you want the non-polite version ask me offline,
preferably near a beer tap and far from people who are easily offended.
Have a nice day,
Martin
[1] Why don't you have that as a word already? Someone was trying to suggest
that:
https://www.collinsdictionary.com/submission/4813/alibistic
Maybe because some other languages have it already:
https://en.wiktionary.org/wiki/alibistick%C3%BD
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/31/2018 03:24 AM, Martin Kletzander wrote:
> On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>>
>> This is way too sparse.
>>
>
> I can't really think of what to say here that is not already mentioned
> in the
> documentation or self-explanatory in the code. But maybe only I see
> that as
> self-explanatory. I'll write something up here.
>
The whole default thing is what really drew my attention back to the
top... It's where I expected to see an explanation why the default was
being set. I think that's something that should be described - you may
disagree, but that's why it's a review.
[...]
See, I know how to cut - I just forgot in my response to patch 4 ;-)
I'll try not to overcut as I've seen done in for reviews I've received
which means I have to find the context of the feedback (we all have our
favorite things to gripe about).
>>> +
>>> +static void
>>> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
>>> + const virDomainDef *def)
>>> +{
>>> + if (!def->tseg_size)
>>
>> Since it's not bool or pointer, prefer the tseg_size == 0
>>
>
> I don't know if you mean that as indicative or imperative. It is very
> subjective and for such scenarios I would like to execute my right to
> mention
> that it is not part of Contributor Guidelines. I know it might seem
> rude, but
> this is one of the things that's very subjective and for such things I
> like to
> first reach a consensus before I change what I'm used to writing. I
> hope you
> understand that.
>
There's a lot of things not specifically listed in the CG that are
mentioned in many reviews or that are just now done because they have
been mentioned (2 blank lines between functions, formatting of function
headers, 1 variable per line, etc.). When I see this style and remember
to note it, I do... And I have see others mention it too. Many times
it's for enum comparisons.
In the long run, whatever. It's a style preference that denotes variable
usage for those reading code "in the future". We don't have a rule for
it, so go with your style if that's what you prefer.
FWIW: back in the dark ages when I first started doing this... Something
like the above for an integer, long, etc. value would be converted by
the compiler to checking *only* if the low bit was set/cleared (think
little endian) because that instruction was really quick. Try to find
that needle in a haystack for each even value that could be used ;-). I
got very used to the explicit comparison to 0 just to be 100% clear.
>>> + return;
>>> +
>>> + virCommandAddArg(cmd, "-global");
>>> +
[...]
>>> + /* QEMU started defaulting to 16MiB after 2.9 */
>>> + if (major > 2 || (major == 2 && minor > 9))
>>> + def->tseg_size = 16 * 1024 * 1024;
>>
>> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>>
>
> TL;DR because not setting the default value when it is not explicitly
> requested has already bitten us in the back multiple times.
>
> Long story short this way we are safe. No matter what happens (QEMU
> changing the default, the user changing the config somewhere else and
> not expecting this to change, us wanting to change the default in the
> future for some reason, migration to newer libvirt where who-knows-what
> is checked there, etc.) it is way easier to keep the guest ABI stable.
> It is visible what the value actually is and we're not hiding it
> somewhere in the code of the hypervisor. I know we don't do that for
> many things. I could've gone with the (often alibistic [1]) "the
> default is left for hypervisor to decide", but since we have the
> opportunity tomake things right (thanks to Laszlo's explanations), why
> shouldn't we?
>
>> This seems to me we are setting policy which based on history of many
>> patches before is a no go. I'd say NAK to this, unless there is some
>> convincing argument made in the commit message and followup responses to
>> the series (or you can take Jan's R-By and ignore me - your call.
>>
>
> What patches are you referring to? I can add that to the commit
> message, sure.
>
In my mind, for starters what I mentioned in my response to patch 3 for
formatdomain.html.in changes.
Beyond that, I don't really don't have the patience to go through
hundreds of patches of history in order to pick out ones that were
NACK'd or requested to be changed because the patch made some policy
decision or a let's set a default value type decision. I know they've
happened though.
TL;DR: I'm still of the opinion we don't set (or even save) a default.
I would think someone that failed to boot their guest because the memory
size or vCPU count was too large for smm/tseg would then be pointed to
this value which they would then optionally add and then "manage" going
forward.
I still don't see the point of setting a default if the default for QEMU
has been good enough up to now up to and including a certain memory size
of number of vCPU's, then how does us setting that default change
anything? Especially if it's the same value that QEMU uses. How do we
know that the value in the XML is one we provided or one the user
provided? Without providing a default and if QEMU changed the default
would a migration fail? If we set a default and then the migration
failed, where does the fickle finger of fate fall? Yes, Laszlo's
explanations are great, but they're lost to the mailing list and the
fading memories of those reading them if not captured in some way in
comments to code.
If there's a consensus to allow a default to be listed, then I'm not
going to even try to block. However, not mentioning it or calling it out
as part of the review would to me would be wrong. I believe by doing
this you're setting a precedent although I could be wrong as I don't
have the entire code base memorized.
>>> +
>>> + return 0;
>>> +
>>> + error:
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("Cannot parse QEMU machine type version '%s'"),
>>> + def->os.machine);
>>> + return -1;
>>> +}
>>> +
>>> +
>>> +static int
>>> +qemuDomainDefTsegPostParse(virDomainDefPtr def,
>>
>> While TSEG is what you're adjusting, this is more a 'features' or in
>> particular SMM features post parse callback.
>>
>
> I'm getting the hint of a hidden suggestion from the sentence. Are you
> suggesting I should rename the function or create few more layers like
> this?
>
> qemuDomainDefSMMFeaturesPostParse() {
> qemuDomainDefTsegPostParse();
> }
>
> qemuDomainDefFeaturesPostParse() {
> qemuDomainDefSMMFeaturesPostParse();
> }
>
> qemuDomainDefPostParse() {
> ...
> qemuDomainDefFeaturesPostParse();
> ...
> }
>
While the above is the progression I had in mind, I cannot make my mind
up if we should go through the insanity of all that or just go with the
qemuDomainDefSMMFeaturesPostParse which "allows" someone with a future
need to create a qemuDomainDefXXXFeaturesPostParse that is then either
directly called from qemuDomainDefPostParse or indirectly from a new
qemuDomainDefFeaturesPostParse.
This another one of those things not explicitly stated in the CG that I
know I've received feedback on in the past or have seen done by other
patches with respect to the naming of functions and/or creation of shell
methods to drill down into the path of the details. Function naming can
be so subjective...
>>> + virQEMUCapsPtr qemuCaps)
>>> +{
>>> + if (def->features[VIR_DOMAIN_FEATURE_SMM] !=
>>> VIR_TRISTATE_SWITCH_ON)
>>> + return 0;
>>> +
>>> + if (!def->tseg_size)
>>
>> Similar... prefer tseg_size == 0
>>
>
> Again, are you directing me to do that or saying what version you like
> more?
>
Pointing out a similar construct that IMO should be changed, but you
seem to dislike that way of comparison, so whatever. Yes, I know it's
functionally equivalent.
>>> + return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>>> +
>>> + if (!qemuDomainIsQ35(def)) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("SMM TSEG is only supported with q35
>>> machine type"));
>>> + return -1;
>>> + }
>>> +
>>> + if (!virQEMUCapsGet(qemuCaps,
>>> QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("Setting TSEG size is not supported with
>>> this QEMU binary"));
>>> + return -1;
>>> + }
>>> +
>>> + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("SMM TSEG size must be divisible by 1 MiB"));
>>> + return -1;
>>> + }
>>
>> Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother.
>>
>> Curious that this differs from qemuDomainMemoryDeviceAlignSize which
>> claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which
>> returns 1024 unless it's PPC64 which returns 256MiB alignments.
>
> It does. The extended TSEG explicitly allows 1 MiB increments. Not
> only because of the naming (*_mbytes), but also because bigger
> granularity would just be pointless probably. Also this is not going to
> exist on PPC64, it only makes sense with q35.
>
I was partially commenting on why even bother - just force the rounding
and indicate that libvirt will do so - that I think has been done
numerous times before. Failing just because someone (tester) used
12345KiB or 1025KiB just seems pointless especially if we can and say we
will round if you don't know how to make things 1MiB aligned. A value
below 1 MiB would be useless - in fact it seems from the default
discussion that anything below 16 MiB would be useless, wouldn't it?
As for the rest - I went and checked a few other VIR_ROUND_UP consumers
and they were claiming the 1MiB rounding, but using 1024 instead of
1024*1024 like is done above which I just found "curious" - I didn't
bother to check the basis of the value being stored, but I think for
memory it is KB.
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
[...]
>>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
>>> @@ -0,0 +1,23 @@
>>> +<domain type='qemu'>
>>> + <name>QEMUGuest1</name>
>>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>>> + <memory unit='KiB'>219100</memory>
>>> + <currentMemory unit='KiB'>219100</currentMemory>
>>> + <vcpu placement='static'>1</vcpu>
>>> + <os>
>>> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
>>> + <boot dev='hd'/>
>>> + </os>
>>> + <features>
>>> + <smm state='on'>
>>> + <tseg>48</tseg>
>>
>> The only difference here from genericxml2xmlindata/tseg.xml is that this
>> one doesn't have "unit='MiB'"
>>
>
> Yes, I wanted to check for the correct parsing of both. Maybe I should
> put 1 GiB in the other one so that I check that it parses the unit
> properly? Actually I have KiB somewhere, so probably not.
>
Considering you commented recently on another series regarding whether
the code should use genericxml2xml - I'm just pointing out the IMO
excess of having both generic and qemu tests doing pretty much the same
thing... You can do what you want though - if both are left there,
that's fine.
>>> + </smm>
>>> + </features>
>>> + <clock offset='utc'/>
>>> + <on_poweroff>destroy</on_poweroff>
>>> + <on_reboot>restart</on_reboot>
>>> + <on_crash>destroy</on_crash>
>>> + <devices>
>>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>>> + </devices>
>>> +</domain>
[...]
>>> --- /dev/null
>>> +++ b/tests/qemuxml2argvdata/tseg.xml
>>
>> and yet another one similr to genericxml2xmlindata/tseg.xml - same name,
>> different directory
>>
>
> Are you suggesting I create this as a symlink? Or that I should add
> info to the commit message that this patch adds checks for formatting
> the command-line (the functionality the patch is adding) and that the
> genericxml2xmltest is checking the parsing even for developers who build
> with QEMU driver disabled?
>
IMO: More duplication of the same test... I'm not the first person to
ever point out duplication of tests in commits. "Some day" maybe
qemuxml2xmltest will be able to source from genericxml2xml - then it's
up to someone to decide which test to use.
[...]
>>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>>> index 78454acb1a41..633dfaaee9a4 100644
>>> --- a/tests/qemuxml2argvtest.c
>>> +++ b/tests/qemuxml2argvtest.c
>>> @@ -2827,6 +2827,54 @@ mymain(void)
>>>
[...]
>>> + DO_TEST_PARSE_ERROR("tseg-i440fx",
>>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>>> + QEMU_CAPS_DEVICE_IOH3420,
>>> + QEMU_CAPS_ICH9_AHCI,
>>> + QEMU_CAPS_MACHINE_SMM_OPT,
>>> + QEMU_CAPS_VIRTIO_SCSI,
>>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>> Failure wrong machine type...
>>
>>> + DO_TEST_PARSE_ERROR("tseg-explicit-size",
>>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>>> + QEMU_CAPS_DEVICE_IOH3420,
>>> + QEMU_CAPS_ICH9_AHCI,
>>> + QEMU_CAPS_MACHINE_SMM_OPT,
>>> + QEMU_CAPS_VIRTIO_SCSI);
>>
>> Failure because TSEG_MBYTES not provided.
>>
>
> I'm sorry, don't take this personally, but sometimes I'm not sure
> whether you are trying to convey some information or you are just adding
> comments for yourself. If you are asking me to do something, then
> please say so. If you want these messages to be there as comments, let
> me know, I'll do that (although the only time that would be useful is if
> someone makes a change and the tests start failing (by the tested code
> not failing) and that person wants to know what was supposed to be the
> error. In which case they can re-run them without the modifications.
> Or if you are suggesting that it should be DO_TEST_FAILURE instead of
> DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the
> code doesn't throw an error during starting the domain, but right away
> during parsing. And yes, that can be done because I'm not erroring out
> for something that existed before, but for a new feature so it will
> never be read from the disk for a domain that existed before this
> series. Or maybe I ran the tests wrong and it is supposed to be
> DO_TEST_FAILURE and I made a mistake somewhere else as well.
>
> The reason for me not really being sure what you mean by that is that I
> take all of what's in the previous paragraph as something obvious and
> known by default for non-newbie libvirt developers (which you most
> certainly are one since you have years of experience in this codebase),
> so I'm probably just missing something. It very well might be a
> language barrier, because I'm nowhere near understanding most of the
> nuances in English language and various dialects thereof. So with no
> hidden meaning between the lines, please enlighten me.
>
Nothing personal... and valid question/points. There's no hidden meaning
- sometimes it's just notes for myself that I forget to go back and
remove and other times it's just validation of what's being tested. Part
of my review processing is going from the test to the code and
considering the error being tested. Just like some people forget to
remove debugging logic from their patches before posting, I just forgot
to remove my review debugging notes - mea culpa and apologies for any
added stress/anxiety ;-). Since Jan had already provided an R-By for
the patch - I figured anything I'm providing becomes feedback for you to
pick and choose which you find particularly necessary to incorporate and
which to ignore.
After a few years of getting reviews I've learned the styles of various
reviewers and what feedback can be ignored, heeded, and challenged.
You've been working on libvirt longer than me, so I'm sure you have
certain filters set already too.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 31, 2018 at 09:45:04AM -0400, John Ferlan wrote:
>
>
>On 05/31/2018 03:24 AM, Martin Kletzander wrote:
>> On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>>>
>>> This is way too sparse.
>>>
>>
>> I can't really think of what to say here that is not already mentioned
>> in the
>> documentation or self-explanatory in the code. But maybe only I see
>> that as
>> self-explanatory. I'll write something up here.
>>
>
>The whole default thing is what really drew my attention back to the
>top... It's where I expected to see an explanation why the default was
>being set. I think that's something that should be described - you may
>disagree, but that's why it's a review.
>
>[...]
>
>See, I know how to cut - I just forgot in my response to patch 4 ;-)
>I'll try not to overcut as I've seen done in for reviews I've received
>which means I have to find the context of the feedback (we all have our
>favorite things to gripe about).
>
>>>> +
>>>> +static void
>>>> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
>>>> + const virDomainDef *def)
>>>> +{
>>>> + if (!def->tseg_size)
>>>
>>> Since it's not bool or pointer, prefer the tseg_size == 0
>>>
>>
>> I don't know if you mean that as indicative or imperative. It is very
>> subjective and for such scenarios I would like to execute my right to
>> mention
>> that it is not part of Contributor Guidelines. I know it might seem
>> rude, but
>> this is one of the things that's very subjective and for such things I
>> like to
>> first reach a consensus before I change what I'm used to writing. I
>> hope you
>> understand that.
>>
>
>There's a lot of things not specifically listed in the CG that are
>mentioned in many reviews or that are just now done because they have
>been mentioned (2 blank lines between functions, formatting of function
>headers, 1 variable per line, etc.). When I see this style and remember
>to note it, I do... And I have see others mention it too. Many times
>it's for enum comparisons.
>
I feel the same way about most of those things. I have some unfinished patches
here and there for adding these to the CG so that we don't have that many
subjective discussions. Having said that I don't mind having the discussions.
I just don't like having some of the discussions multiple times =) I will learn
a new way if there is a clearly written consensus, it's just that I don't want
to do that just based on some reviews. Until it is written in stone^Wthe repo,
I'm not that convinced. But what is a great plus for me, here and now, is that
I understand your review more. I wasn't really sure whether that was optional
feedback or a requirement before pushing =) Thanks for the explanation. That's
why it is very mandatory to have a healthy communication.
>In the long run, whatever. It's a style preference that denotes variable
>usage for those reading code "in the future". We don't have a rule for
>it, so go with your style if that's what you prefer.
>
>FWIW: back in the dark ages when I first started doing this... Something
>like the above for an integer, long, etc. value would be converted by
>the compiler to checking *only* if the low bit was set/cleared (think
>little endian) because that instruction was really quick. Try to find
>that needle in a haystack for each even value that could be used ;-). I
>got very used to the explicit comparison to 0 just to be 100% clear.
>
>>>> + return;
>>>> +
>>>> + virCommandAddArg(cmd, "-global");
>>>> +
>
>[...]
>
>>>> + /* QEMU started defaulting to 16MiB after 2.9 */
>>>> + if (major > 2 || (major == 2 && minor > 9))
>>>> + def->tseg_size = 16 * 1024 * 1024;
>>>
>>> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>>>
>>
>> TL;DR because not setting the default value when it is not explicitly
>> requested has already bitten us in the back multiple times.
>>
>> Long story short this way we are safe. No matter what happens (QEMU
>> changing the default, the user changing the config somewhere else and
>> not expecting this to change, us wanting to change the default in the
>> future for some reason, migration to newer libvirt where who-knows-what
>> is checked there, etc.) it is way easier to keep the guest ABI stable.
>> It is visible what the value actually is and we're not hiding it
>> somewhere in the code of the hypervisor. I know we don't do that for
>> many things. I could've gone with the (often alibistic [1]) "the
>> default is left for hypervisor to decide", but since we have the
>> opportunity tomake things right (thanks to Laszlo's explanations), why
>> shouldn't we?
>>
>>> This seems to me we are setting policy which based on history of many
>>> patches before is a no go. I'd say NAK to this, unless there is some
>>> convincing argument made in the commit message and followup responses to
>>> the series (or you can take Jan's R-By and ignore me - your call.
>>>
>>
>> What patches are you referring to? I can add that to the commit
>> message, sure.
>>
>
>In my mind, for starters what I mentioned in my response to patch 3 for
>formatdomain.html.in changes.
>
>Beyond that, I don't really don't have the patience to go through
>hundreds of patches of history in order to pick out ones that were
>NACK'd or requested to be changed because the patch made some policy
>decision or a let's set a default value type decision. I know they've
>happened though.
>
>TL;DR: I'm still of the opinion we don't set (or even save) a default.
>
See:
- commit 8dc88aeed612aeea1ae0d249ec760938cecce5aa
- commit bf20251048534022efe21785f98949c772ce7a71
- function qemuDomainGetSCSIControllerModel
- Andrea's issues with default GIC version (many commits)
- commit f55eaccb0c570767d8245f57deae188255ee995e
and I'm sure there's more. Even ones that don't only differentiate on the
architecture.
>I would think someone that failed to boot their guest because the memory
>size or vCPU count was too large for smm/tseg would then be pointed to
>this value which they would then optionally add and then "manage" going
>forward.
>
>I still don't see the point of setting a default if the default for QEMU
>has been good enough up to now up to and including a certain memory size
>of number of vCPU's, then how does us setting that default change
>anything? Especially if it's the same value that QEMU uses. How do we
>know that the value in the XML is one we provided or one the user
>provided? Without providing a default and if QEMU changed the default
>would a migration fail? If we set a default and then the migration
>failed, where does the fickle finger of fate fall? Yes, Laszlo's
>explanations are great, but they're lost to the mailing list and the
>fading memories of those reading them if not captured in some way in
>comments to code.
>
>If there's a consensus to allow a default to be listed, then I'm not
>going to even try to block. However, not mentioning it or calling it out
>as part of the review would to me would be wrong. I believe by doing
>this you're setting a precedent although I could be wrong as I don't
>have the entire code base memorized.
>
I'll see how the discussion goes with Pavel in the other thread, if someone
chimes in. This patch is in no rush since it's too close to release anyway, so
we'll see. I'm okay with setting it to whatever was the default during the first
start (getting the value from QEM) for example. Or maybe leaving the code
altogether (even though I must admit it's partially because with this mail
history I will be able to say "told you so" in case the day comes like 5 years
from now).
>>>> +
>>>> + return 0;
>>>> +
>>>> + error:
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("Cannot parse QEMU machine type version '%s'"),
>>>> + def->os.machine);
>>>> + return -1;
>>>> +}
>>>> +
>>>> +
>>>> +static int
>>>> +qemuDomainDefTsegPostParse(virDomainDefPtr def,
>>>
>>> While TSEG is what you're adjusting, this is more a 'features' or in
>>> particular SMM features post parse callback.
>>>
>>
>> I'm getting the hint of a hidden suggestion from the sentence. Are you
>> suggesting I should rename the function or create few more layers like
>> this?
>>
>> qemuDomainDefSMMFeaturesPostParse() {
>> qemuDomainDefTsegPostParse();
>> }
>>
>> qemuDomainDefFeaturesPostParse() {
>> qemuDomainDefSMMFeaturesPostParse();
>> }
>>
>> qemuDomainDefPostParse() {
>> ...
>> qemuDomainDefFeaturesPostParse();
>> ...
>> }
>>
>
>While the above is the progression I had in mind, I cannot make my mind
>up if we should go through the insanity of all that or just go with the
>qemuDomainDefSMMFeaturesPostParse which "allows" someone with a future
>need to create a qemuDomainDefXXXFeaturesPostParse that is then either
>directly called from qemuDomainDefPostParse or indirectly from a new
>qemuDomainDefFeaturesPostParse.
>
>This another one of those things not explicitly stated in the CG that I
>know I've received feedback on in the past or have seen done by other
>patches with respect to the naming of functions and/or creation of shell
>methods to drill down into the path of the details. Function naming can
>be so subjective...
>
>
>>>> + virQEMUCapsPtr qemuCaps)
>>>> +{
>>>> + if (def->features[VIR_DOMAIN_FEATURE_SMM] !=
>>>> VIR_TRISTATE_SWITCH_ON)
>>>> + return 0;
>>>> +
>>>> + if (!def->tseg_size)
>>>
>>> Similar... prefer tseg_size == 0
>>>
>>
>> Again, are you directing me to do that or saying what version you like
>> more?
>>
>
>Pointing out a similar construct that IMO should be changed, but you
>seem to dislike that way of comparison, so whatever. Yes, I know it's
>functionally equivalent.
>
>>>> + return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>>>> +
>>>> + if (!qemuDomainIsQ35(def)) {
>>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>> + _("SMM TSEG is only supported with q35
>>>> machine type"));
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (!virQEMUCapsGet(qemuCaps,
>>>> QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>> + _("Setting TSEG size is not supported with
>>>> this QEMU binary"));
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>> + _("SMM TSEG size must be divisible by 1 MiB"));
>>>> + return -1;
>>>> + }
>>>
>>> Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother.
>>>
>>> Curious that this differs from qemuDomainMemoryDeviceAlignSize which
>>> claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which
>>> returns 1024 unless it's PPC64 which returns 256MiB alignments.
>>
>> It does. The extended TSEG explicitly allows 1 MiB increments. Not
>> only because of the naming (*_mbytes), but also because bigger
>> granularity would just be pointless probably. Also this is not going to
>> exist on PPC64, it only makes sense with q35.
>>
>
>I was partially commenting on why even bother - just force the rounding
>and indicate that libvirt will do so - that I think has been done
>numerous times before. Failing just because someone (tester) used
>12345KiB or 1025KiB just seems pointless especially if we can and say we
>will round if you don't know how to make things 1MiB aligned. A value
>below 1 MiB would be useless - in fact it seems from the default
>discussion that anything below 16 MiB would be useless, wouldn't it?
>
>As for the rest - I went and checked a few other VIR_ROUND_UP consumers
>and they were claiming the 1MiB rounding, but using 1024 instead of
>1024*1024 like is done above which I just found "curious" - I didn't
>bother to check the basis of the value being stored, but I think for
>memory it is KB.
>
Yeah, most of the stuff is kept in KiB, I keep it in Bytes. Reason?
Extensibility in the future just in case some other hypervisor adds similar
option. Also because I imagined that if I kept it in MiB someone would tell me
the opposite :)
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +
>
>[...]
>
>>>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
>>>> @@ -0,0 +1,23 @@
>>>> +<domain type='qemu'>
>>>> + <name>QEMUGuest1</name>
>>>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>>>> + <memory unit='KiB'>219100</memory>
>>>> + <currentMemory unit='KiB'>219100</currentMemory>
>>>> + <vcpu placement='static'>1</vcpu>
>>>> + <os>
>>>> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type>
>>>> + <boot dev='hd'/>
>>>> + </os>
>>>> + <features>
>>>> + <smm state='on'>
>>>> + <tseg>48</tseg>
>>>
>>> The only difference here from genericxml2xmlindata/tseg.xml is that this
>>> one doesn't have "unit='MiB'"
>>>
>>
>> Yes, I wanted to check for the correct parsing of both. Maybe I should
>> put 1 GiB in the other one so that I check that it parses the unit
>> properly? Actually I have KiB somewhere, so probably not.
>>
>
>Considering you commented recently on another series regarding whether
>the code should use genericxml2xml - I'm just pointing out the IMO
>excess of having both generic and qemu tests doing pretty much the same
>thing... You can do what you want though - if both are left there,
>that's fine.
>
They are not doing the same thing. genericxml2xml is testing XML
formatting/parsing in domain_conf. qemuxml2xml is testing the driver-specific
postparse bits and qemuxml2argv is testing the command-line generation.
>>>> + </smm>
>>>> + </features>
>>>> + <clock offset='utc'/>
>>>> + <on_poweroff>destroy</on_poweroff>
>>>> + <on_reboot>restart</on_reboot>
>>>> + <on_crash>destroy</on_crash>
>>>> + <devices>
>>>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>>>> + </devices>
>>>> +</domain>
>
>[...]
>
>>>> --- /dev/null
>>>> +++ b/tests/qemuxml2argvdata/tseg.xml
>>>
>>> and yet another one similr to genericxml2xmlindata/tseg.xml - same name,
>>> different directory
>>>
>>
>> Are you suggesting I create this as a symlink? Or that I should add
>> info to the commit message that this patch adds checks for formatting
>> the command-line (the functionality the patch is adding) and that the
>> genericxml2xmltest is checking the parsing even for developers who build
>> with QEMU driver disabled?
>>
>
>IMO: More duplication of the same test... I'm not the first person to
>ever point out duplication of tests in commits. "Some day" maybe
>qemuxml2xmltest will be able to source from genericxml2xml - then it's
>up to someone to decide which test to use.
>
Yeah, we can do that. The only advantage, though, would be that we keep less
files around. The number of tests would be the same. And we can do that with
symlinks nowadays. So probably that's why nobody bothered.
>[...]
>
>>>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>>>> index 78454acb1a41..633dfaaee9a4 100644
>>>> --- a/tests/qemuxml2argvtest.c
>>>> +++ b/tests/qemuxml2argvtest.c
>>>> @@ -2827,6 +2827,54 @@ mymain(void)
>>>>
>
>[...]
>
>>>> + DO_TEST_PARSE_ERROR("tseg-i440fx",
>>>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>>>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>>>> + QEMU_CAPS_DEVICE_IOH3420,
>>>> + QEMU_CAPS_ICH9_AHCI,
>>>> + QEMU_CAPS_MACHINE_SMM_OPT,
>>>> + QEMU_CAPS_VIRTIO_SCSI,
>>>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
>>> Failure wrong machine type...
>>>
>>>> + DO_TEST_PARSE_ERROR("tseg-explicit-size",
>>>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>>>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>>>> + QEMU_CAPS_DEVICE_IOH3420,
>>>> + QEMU_CAPS_ICH9_AHCI,
>>>> + QEMU_CAPS_MACHINE_SMM_OPT,
>>>> + QEMU_CAPS_VIRTIO_SCSI);
>>>
>>> Failure because TSEG_MBYTES not provided.
>>>
>>
>> I'm sorry, don't take this personally, but sometimes I'm not sure
>> whether you are trying to convey some information or you are just adding
>> comments for yourself. If you are asking me to do something, then
>> please say so. If you want these messages to be there as comments, let
>> me know, I'll do that (although the only time that would be useful is if
>> someone makes a change and the tests start failing (by the tested code
>> not failing) and that person wants to know what was supposed to be the
>> error. In which case they can re-run them without the modifications.
>> Or if you are suggesting that it should be DO_TEST_FAILURE instead of
>> DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the
>> code doesn't throw an error during starting the domain, but right away
>> during parsing. And yes, that can be done because I'm not erroring out
>> for something that existed before, but for a new feature so it will
>> never be read from the disk for a domain that existed before this
>> series. Or maybe I ran the tests wrong and it is supposed to be
>> DO_TEST_FAILURE and I made a mistake somewhere else as well.
>>
>> The reason for me not really being sure what you mean by that is that I
>> take all of what's in the previous paragraph as something obvious and
>> known by default for non-newbie libvirt developers (which you most
>> certainly are one since you have years of experience in this codebase),
>> so I'm probably just missing something. It very well might be a
>> language barrier, because I'm nowhere near understanding most of the
>> nuances in English language and various dialects thereof. So with no
>> hidden meaning between the lines, please enlighten me.
>>
>
>Nothing personal... and valid question/points. There's no hidden meaning
>- sometimes it's just notes for myself that I forget to go back and
>remove and other times it's just validation of what's being tested. Part
>of my review processing is going from the test to the code and
>considering the error being tested. Just like some people forget to
>remove debugging logic from their patches before posting, I just forgot
>to remove my review debugging notes - mea culpa and apologies for any
>added stress/anxiety ;-). Since Jan had already provided an R-By for
>the patch - I figured anything I'm providing becomes feedback for you to
>pick and choose which you find particularly necessary to incorporate and
>which to ignore.
>
That's perfectly fine, I just wasn't ever sure whether I understand it corectly.
>After a few years of getting reviews I've learned the styles of various
>reviewers and what feedback can be ignored, heeded, and challenged.
>You've been working on libvirt longer than me, so I'm sure you have
>certain filters set already too.
>
I'm different in this, I guess. I think the review should be readable for all
people the same way, no matter whether you know the person, what they are used
to say or how they mean things. That way it's unambiguous. OTOH I'm not saying
that I'm doing everything the best way and that my opinions are the best, so
it's time for me to learn some new ways! ;)
>John
>
>[...]
Have a nice day,
Martin--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.