On 11/22/21 10:30, Peter Krempa wrote:
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@redhat.com> Tested-by: Kashyap Chamarthy <kchamart@redhat.com> --- 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.
Fair enough. I don't have any evidence. Let me remove the whole function.
+ 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.
I'm not sure what you mean. If value 0 is passed then the parser won't set def->features[VIR_DOMAIN_FEATURE_TCG] so this function exits early. Do you want me to put if (val > 0) check here or something different?
+ + 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.
Fair enough. I thought that in this case it borderline okay, but I don't care that much really. Michal