[libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
Martin Kletzander
mkletzan at redhat.com
Thu May 31 07:24:39 UTC 2018
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180531/680f7e29/attachment-0001.sig>
More information about the libvir-list
mailing list