On Fri, Nov 05, 2021 at 10:35:19 +0100, Michal Privoznik wrote:
> It may come handy to be able to tweak TCG options, in this
> specific case the size of translation block cache size (tb-size).
> Since we can expect more knobs to tweak let's put them under
> common element, like this:
>
> <domain>
> <features>
> <tcg>
> <tb-cache unit='MiB'>128</tb-cache>
> </tcg>
> </features>
> </domain>
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> Tested-by: Kashyap Chamarthy <kchamart@xxxxxxxxxx>
> ---
> docs/formatdomain.rst | 11 +++
> docs/schemas/domaincommon.rng | 15 +++-
> src/conf/domain_conf.c | 90 +++++++++++++++++++
> src/conf/domain_conf.h | 7 ++
> src/qemu/qemu_validate.c | 11 +++
> .../x86_64-default-cpu-tcg-features.xml | 67 ++++++++++++++
> ...default-cpu-tcg-features.x86_64-latest.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 8 files changed, 202 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
> create mode 120000 tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-features.x86_64-latest.xml
[...]
> @@ -21555,6 +21585,39 @@ virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDef *src,
> }
>
>
> +static bool
> +virDomainFeatureTCGCheckABIStability(const virDomainDef *src,
> + const virDomainDef *dst)
> +{
> + const int srcF = src->features[VIR_DOMAIN_FEATURE_TCG];
> + const int dstF = dst->features[VIR_DOMAIN_FEATURE_TCG];
> +
> + if (srcF != dstF ||
> + !!src->tcg_features != !!dst->tcg_features) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("State of feature '%s' differs: "
> + "source: '%s', destination: '%s'"),
> + virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_TCG),
> + virTristateSwitchTypeToString(srcF),
> + virTristateSwitchTypeToString(dstF));
> + return false;
> + }
> +
> + if (!src->tcg_features && !dst->tcg_features)
> + return true;
This check is either questionable (e.g. if just one of them is non-NULL,
this doesn't trigger and the subsequent condition dereferences NULL in
the other one), or unnecessary.
> + if (src->tcg_features->tb_cache != dst->tcg_features->tb_cache) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("TCG tb-cache mismatch: source %llu, destination: %llu"),
> + src->tcg_features->tb_cache,
> + dst->tcg_features->tb_cache);
I don't think this is ABI, do you have any supporting evidence? If yes,
put it into the commnet for the next person questioning this.
> + return false;
> + }
> +
> + return true;
> +}
> +
> +
> static bool
> virDomainDefFeaturesCheckABIStability(virDomainDef *src,
> virDomainDef *dst)
[...]
> @@ -27691,6 +27759,24 @@ virDomainDefFormatBlkiotune(virBuffer *buf,
> }
>
>
> +static void
> +virDomainFeatureTCGFormat(virBuffer *buf,
> + const virDomainDef *def)
> +{
> + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
> +
> + if (!def->tcg_features ||
> + def->features[VIR_DOMAIN_FEATURE_TCG] != VIR_TRISTATE_SWITCH_ON)
> + return;
> +
> + virBufferAsprintf(&childBuf,
> + "<tb-cache unit='KiB'>%lld</tb-cache>\n",
> + def->tcg_features->tb_cache);
This is not very extensible and similarly the parser as setting the
cache to 0 is considered as not being present.
> +
> + virXMLFormatElement(buf, "tcg", NULL, &childBuf);
> +}
> +
> +
> static int
> virDomainDefFormatFeatures(virBuffer *buf,
> virDomainDef *def)
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 397eea5ede..bd33c9a800 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -294,6 +294,17 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
> }
> break;
>
> + case VIR_DOMAIN_FEATURE_TCG:
> + if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> + if (def->virtType != VIR_DOMAIN_VIRT_QEMU) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("TCG features are incompatible with domain type '%s'"),
> + virDomainVirtTypeToString(def->virtType));
> + return -1;
> + }
> + }
> + break;
> +
Preferably, implement the qemu logic in the patch adding the qemu bits.
> case VIR_DOMAIN_FEATURE_SMM:
> case VIR_DOMAIN_FEATURE_KVM:
> case VIR_DOMAIN_FEATURE_XEN:
> diff --git a/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
> new file mode 100644
> index 0000000000..e2058487b2
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
> @@ -0,0 +1,67 @@
> +<domain type='qemu'>
> + <name>guest</name>
> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
> + <memory unit='KiB'>4194304</memory>
> + <currentMemory unit='KiB'>4194304</currentMemory>
> + <vcpu placement='static'>4</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-q35-6.2'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <acpi/>
> + <apic/>
> + <tcg>
> + <tb-cache unit='KiB'>102400</tb-cache>
> + </tcg>
> + </features>
> + <cpu mode='custom' match='exact' check='none'>
> + <model fallback='forbid'>qemu64</model>
> + </cpu>
> + <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>
> + <disk type='file' device='disk'>
> + <driver name='qemu' type='qcow2'/>
> + <source file='/var/lib/libvirt/images/guest.qcow2'/>
> + <target dev='vda' bus='virtio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
> + </disk>
Storage is not relevant to the test. Please remove the disk from this
definition.
