On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> TSEG (Top of Memory Segment) is one of many regions that SMM (System Management
> Mode) can occupy. This one, however is special, because a) most of the SMM code
> lives in TSEG nowadays and b) QEMU just (well, some time ago) added support for
> so called 'extended' TSEG. The difference to the TSEG implemented in real q35's
> MCH (Memory Controller Hub) is that it can have any size from 1 MiB up to 65534
> MiB in 1 MiB increments. But more about that in the QEMU patch.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Which some reader, but not this one, may be eager to find ;-)
Still is there a valid range QEMU would accept? Or do we just let QEMU
fail if the range is too high?
I think QEMU has MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX
>
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 39 +++++++++++++++++++
> docs/schemas/domaincommon.rng | 5 +++
> src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 1 +
> tests/genericxml2xmlindata/tseg.xml | 23 +++++++++++
> tests/genericxml2xmltest.c | 2 +
> 6 files changed, 129 insertions(+), 1 deletion(-)
> create mode 100644 tests/genericxml2xmlindata/tseg.xml
>
In the category of I hate it when that happens, git am -3 "merged" in
two chunks incorrectly! Probably wouldn't have happened if I'd done
this sooner! The virDomainDefFeaturesCheckABIStability hunk got merged
into virDomainRedirFilterDefCheckABIStability and the tseg grammar got
merged under "vmport" and just before "gic". As you can imagine the
results weren't pretty ;-).
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 403b638bd4bd..39ebfe398bd7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1897,6 +1897,9 @@
> <ioapic driver='qemu'/>
> <hpt resizing='required'/>
> <vmcoreinfo state='on'/>
> + <smm state='on'>
> + <tseg unit='MiB'>48</tseg>
> + </smm>
> </features>
> ...</pre>
>
> @@ -2056,6 +2059,42 @@
> <code>off</code>, default <code>on</code>) enable or disable
> System Management Mode.
> <span class="since">Since 2.1.0</span>
> +
> + Optional sub-element <code>tseg</code> can be used to specify the
> + amount of memory dedicated to SMM TSEG. The size can be specified as a
> + value of that element, optional attribute <code>unit</code> can be
> + used to specify the unit of the aforementioned value (defaults to
> + 'MiB').
> +
If this is to be a true paragraph break then these paragraphs needs to
be wrapped in <p> ... </p>; otherwise, this just becomes one long run on
(and quite ugly IMO) paragraph.
> + This value is configurable due to the fact that the calculation cannot
> + be done right with the guarantee that it will work correctly. For
> + QEMU TSEG was disabled up to and including <code>pc-q35-2.9</code> (it
> + does not make sense fo any other machine type than q35).
s/fo/for/
This also appears to be a paragraph break...
> + From <code>pc-q35-2.10</code> the default value was changed to 16 MiB.
s/From/Starting with/ ??? (not required, just a though)
> + That should suffice for up to 272 VCPUs, 5 GiB guest RAM in total, no
> + hotplug memory range, and 32 GiB of 64-bit PCI MMIO aperture. Or for
> + 48 VCPUs, with 1TB of guest RAM, no hotplug DIMM range, and 32GB of
> + 64-bit PCI MMIO aperture. The values may also vary based on the loader
> + the VM is using.
> +
> + Additional size might be needed for significantly higher VCPU counts
> + or increased address space (that can be memory, maxMemory, 64-bit PCI
> + MMIO aperture size; roughly 8 MiB of TSEG per 1 TiB of address space)
> + which can also be rounded up.
Uh, oh, hmmm... We seem to have this (perhaps more recent) "thing" about
libvirt having to supply some attribute based on some (mostly difficult
to describe) algorithm that QEMU would use in order to create the
"optimum" size or use for some alternate algorithm. Of course, a few
libvir-list patches like that have been NACK'd because of the feeling
that providing a useful algorithm for a user to "decide upon" a
satisfactory attribute value cannot really be done. Off the top of my
head I can come up with two:
1. Add poll-max-ns property of each iothread:
https://www.redhat.com/archives/libvir-list/2017-February/msg01047.html
2. Add support for qcow2 cache (many times, but most recently):
https://www.redhat.com/archives/libvir-list/2017-September/msg00553.html
> +
> + Due to the nature of this setting being similar to "how much RAM
> + should the guest have" users are advised to either consult the
> + documentation of the guest OS or loader (if there is any), or test
> + this by trial-and-error changing the value until the VM boots
> + successfully. Yet another guiding value for users might be the fact
> + that 48 MiB should be enough for pretty large guests (240 VCPUs and
> + 4TB guest RAM), but it is on purpose not set as default as 48 MiB of
> + unavailable RAM might be too much for small guests (e.g. with 512 MiB
> + of RAM).
and this is the exact reason why patches like this get NACKd - because
trial and error should not be a 'desired' means to calculate. Even the
bz referenced in patch 5 has an incredible amount of data and
calculations that provide even more insight and details that are lost
when we try to summarize in a libvirt meaningful patch.
What it seems is really needed is an attribute that libvirt provides
that tells QEMU to calculate the optimum size.
> +
> + See <a href="#elementsMemoryAllocation">Memory Allocation</a>
> + for more details about the <code>unit</code> attribute.
> + <span class="since">Since 4.5.0</span> (QEMU only)
haha - see you put 4.5.0 and this is the 4.4.0 release - so it was
ignored until 4.5.0 was "on the clock" ;-)
Ironically this is pointed out as QEMU only; however, genericxml2xmltest
is used/updated.
So, I personally don't mind if this attribute is added; however, I think
we become hypocrites to a certain degree if patches continue to be
blocked/NACKed to other subsystems that have attributes that allow
certain amount of control, but don't come with exact sizing references.
Still if this is pushed, then perhaps those others can use this as the
means to provide a reference to other knobs added.
You can have my :
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
with a few adjustments above; however, another R-By should be obtained
here as well as perhaps a policy change so that other similar such
series could be merged... I guess I'm curious what "thoughts" others may
have regarding adding this "knob" while not allowing others.
John
> </dd>
> <dt><code>ioapic</code></dt>
> <dd>Tune the I/O APIC. Possible values for the
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 71ac3d079c32..664ec79933f0 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4785,6 +4785,11 @@
> <ref name="virOnOff"/>
> </attribute>
> </optional>
> + <optional>
> + <element name="tseg">
> + <ref name="scaledInteger"/>
> + </element>
> + </optional>
> </element>
> </optional>
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3689ac0a82ce..6a4f8243183d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19647,6 +19647,16 @@ virDomainDefParseXML(xmlDocPtr xml,
> VIR_FREE(nodes);
> }
>
> + if (def->features[VIR_DOMAIN_FEATURE_SMM] == VIR_TRISTATE_SWITCH_ON &&
> + virDomainParseScaledValue("string(./features/smm/tseg)",
> + "string(./features/smm/tseg/@unit)",
> + ctxt,
> + &def->tseg_size,
> + 1024 * 1024, /* Defaults to mebibytes */
> + ULLONG_MAX,
> + false) < 0)
> + goto error;
> +
> if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0)
> goto error;
>
> @@ -21741,6 +21751,23 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
> }
> }
>
> + /* smm */
> + if (src->features[VIR_DOMAIN_FEATURE_SMM] == VIR_TRISTATE_SWITCH_ON &&
> + src->tseg_size != dst->tseg_size) {
> + const char *unit_src, *unit_dst;
> + unsigned long long short_size_src = virFormatIntPretty(src->tseg_size,
> + &unit_src);
> + unsigned long long short_size_dst = virFormatIntPretty(dst->tseg_size,
> + &unit_dst);
> +
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Size of SMM TSEG size differs: "
> + "source: '%llu %s', destination: '%llu %s'"),
> + short_size_src, unit_src,
> + short_size_dst, unit_dst);
> + return false;
> + }
> +
> return true;
> }
>
> @@ -27059,7 +27086,6 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> case VIR_DOMAIN_FEATURE_PMU:
> case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> case VIR_DOMAIN_FEATURE_VMPORT:
> - case VIR_DOMAIN_FEATURE_SMM:
> switch ((virTristateSwitch) def->features[i]) {
> case VIR_TRISTATE_SWITCH_LAST:
> case VIR_TRISTATE_SWITCH_ABSENT:
> @@ -27076,6 +27102,38 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>
> break;
>
> + case VIR_DOMAIN_FEATURE_SMM:
> + switch ((virTristateSwitch) def->features[i]) {
> + case VIR_TRISTATE_SWITCH_LAST:
> + case VIR_TRISTATE_SWITCH_ABSENT:
> + break;
> +
> + case VIR_TRISTATE_SWITCH_ON:
> + virBufferAddLit(buf, "<smm state='on'");
> + if (!def->tseg_size) {
> + virBufferAddLit(buf, "/>\n");
> + } else {
> + const char *unit;
> + unsigned long long short_size = virFormatIntPretty(def->tseg_size,
> + &unit);
> +
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<tseg unit='%s'>%llu</tseg>\n",
> + unit, short_size);
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</smm>\n");
> + }
> +
> + break;
> +
> + case VIR_TRISTATE_SWITCH_OFF:
> + virBufferAddLit(buf, "<smm state='off'/>\n");
> + break;
> + }
> +
> + break;
> +
> case VIR_DOMAIN_FEATURE_APIC:
> if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> virBufferAddLit(buf, "<apic");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a78fdee40c65..c230dd3d09af 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2377,6 +2377,7 @@ struct _virDomainDef {
> virGICVersion gic_version;
> char *hyperv_vendor_id;
> int apic_eoi;
> + unsigned long long tseg_size;
>
> virDomainClockDef clock;
>
> diff --git a/tests/genericxml2xmlindata/tseg.xml b/tests/genericxml2xmlindata/tseg.xml
> new file mode 100644
> index 000000000000..49483f874cd4
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/tseg.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='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/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index d8270a6cae82..daad6e0f78d8 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -141,6 +141,8 @@ mymain(void)
> DO_TEST_FULL("cachetune-colliding-types", false, true,
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
>
> + DO_TEST("tseg");
> +
> virObjectUnref(caps);
> virObjectUnref(xmlopt);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
