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
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index eb8c973cf1..1b7e92b3ba 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1864,6 +1864,9 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
<cfpc value='workaround'/>
<sbbc value='workaround'/>
<ibs value='fixed-na'/>
+ <tcg>
+ <tb-cache unit='MiB'>128</tb-cache>
+ </tcg>
</features>
...
@@ -2065,6 +2068,14 @@ are:
``fixed-na (fixed in hardware - no longer applicable)``. If the
attribute is not defined, the hypervisor default will be used. :since:`Since
6.3.0` (QEMU/KVM only)
+``tcg``
+ Various features to change the behavior of the TCG accelerator.
+
+ =========== ============================================== =================================================== ==============
+ Feature Description Value Since
+ =========== ============================================== =================================================== ==============
+ tb-cache The size of translation block cache size an integer :since:`7.10.0`
+ =========== ============================================== =================================================== ==============
:anchor:`<a id="elementsTime"/>`
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f01b7a6470..ce51e95895 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -6092,7 +6092,7 @@
</element>
</define>
<!--
- A set of optional features: PAE, APIC, ACPI, GIC,
+ A set of optional features: PAE, APIC, ACPI, GIC, TCG,
HyperV Enlightenment, KVM features, paravirtual spinlocks and HAP support
-->
<define name="features">
@@ -6232,6 +6232,9 @@
<optional>
<ref name="ibs"/>
</optional>
+ <optional>
+ <ref name="tcgfeatures"/>
+ </optional>
</interleave>
</element>
</optional>
@@ -6520,6 +6523,16 @@
</element>
</define>
+ <define name="tcgfeatures">
+ <element name="tcg">
+ <optional>
+ <element name="tb-cache">
+ <ref name="scaledInteger"/>
+ </element>
+ </optional>
+ </element>
+ </define>
+
<define name="address">
<element name="address">
<choice>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index da0c64b460..496e43dc02 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -172,6 +172,7 @@ VIR_ENUM_IMPL(virDomainFeature,
"cfpc",
"sbbc",
"ibs",
+ "tcg",
);
VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -17679,6 +17680,30 @@ virDomainFeaturesCapabilitiesDefParse(virDomainDef *def,
}
+static int
+virDomainFeaturesTCGDefParse(virDomainDef *def,
+ xmlXPathContextPtr ctxt,
+ xmlNodePtr node)
+{
+ g_autofree virDomainFeatureTCG *tcg = NULL;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
+
+ tcg = g_new0(virDomainFeatureTCG, 1);
+ ctxt->node = node;
+
+ if (virDomainParseMemory("./tb-cache", "./tb-cache/@unit",
+ ctxt, &tcg->tb_cache, false, false) < 0)
+ return -1;
+
+ if (tcg->tb_cache == 0)
+ return 0;
+
+ def->features[VIR_DOMAIN_FEATURE_TCG] = VIR_TRISTATE_SWITCH_ON;
+ def->tcg_features = g_steal_pointer(&tcg);
+ return 0;
+}
+
+
static int
virDomainFeaturesDefParse(virDomainDef *def,
xmlXPathContextPtr ctxt)
@@ -17887,6 +17912,11 @@ virDomainFeaturesDefParse(virDomainDef *def,
break;
}
+ case VIR_DOMAIN_FEATURE_TCG:
+ if (virDomainFeaturesTCGDefParse(def, ctxt, nodes[i]) < 0)
+ return -1;
+ break;
+
case VIR_DOMAIN_FEATURE_LAST:
break;
}
@@ -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;
+
+ 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);
+ return false;
+ }
+
+ return true;
+}
+
+
static bool
virDomainDefFeaturesCheckABIStability(virDomainDef *src,
virDomainDef *dst)
@@ -21705,6 +21768,11 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
case VIR_DOMAIN_FEATURE_MSRS:
break;
+ case VIR_DOMAIN_FEATURE_TCG:
+ if (!virDomainFeatureTCGCheckABIStability(src, dst))
+ return false;
+ break;
+
case VIR_DOMAIN_FEATURE_LAST:
break;
}
@@ -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);
+
+ virXMLFormatElement(buf, "tcg", NULL, &childBuf);
+}
+
+
static int
virDomainDefFormatFeatures(virBuffer *buf,
virDomainDef *def)
@@ -28011,6 +28097,10 @@ virDomainDefFormatFeatures(virBuffer *buf,
virDomainIBSTypeToString(def->features[i]));
break;
+ case VIR_DOMAIN_FEATURE_TCG:
+ virDomainFeatureTCGFormat(&childBuf, def);
+ break;
+
case VIR_DOMAIN_FEATURE_LAST:
break;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ab9a7d66f8..9c66e26ccf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2054,6 +2054,7 @@ typedef enum {
VIR_DOMAIN_FEATURE_CFPC,
VIR_DOMAIN_FEATURE_SBBC,
VIR_DOMAIN_FEATURE_IBS,
+ VIR_DOMAIN_FEATURE_TCG,
VIR_DOMAIN_FEATURE_LAST
} virDomainFeature;
@@ -2262,6 +2263,11 @@ typedef enum {
VIR_ENUM_DECL(virDomainIBS);
+typedef struct _virDomainFeatureTCG virDomainFeatureTCG;
+struct _virDomainFeatureTCG {
+ unsigned long long tb_cache; /* Stored in KiB */
+};
+
/* Operating system configuration data & machine / arch */
struct _virDomainOSEnv {
char *name;
@@ -2824,6 +2830,7 @@ struct _virDomainDef {
unsigned long long hpt_maxpagesize; /* Stored in KiB */
char *hyperv_vendor_id;
virTristateSwitch apic_eoi;
+ virDomainFeatureTCG *tcg_features;
bool tseg_specified;
unsigned long long tseg_size;
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;
+
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>
+ <controller type='usb' index='0' model='qemu-xhci'>
+ <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+ </controller>
+ <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='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='1' port='0x8'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/>
+ </controller>
+ <controller type='pci' index='2' model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='2' port='0x9'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='3' model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='3' port='0xa'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='4' model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='4' port='0xb'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x3'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <audio id='1' type='none'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-features.x86_64-latest.xml b/tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-features.x86_64-latest.xml
new file mode 120000
index 0000000000..8226a54027
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-features.x86_64-latest.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 7ea066fcda..500386fd00 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1386,6 +1386,7 @@ mymain(void)
DO_TEST_CAPS_ARCH_LATEST("x86_64-default-cpu-tcg-pc-4.2", "x86_64");
DO_TEST_CAPS_ARCH_LATEST("x86_64-default-cpu-kvm-q35-4.2", "x86_64");
DO_TEST_CAPS_ARCH_LATEST("x86_64-default-cpu-tcg-q35-4.2", "x86_64");
+ DO_TEST_CAPS_ARCH_LATEST("x86_64-default-cpu-tcg-features", "x86_64");
DO_TEST_CAPS_LATEST("virtio-9p-multidevs");
DO_TEST_CAPS_LATEST("virtio-9p-createmode");
--
2.32.0
On 11/5/21 10:35 AM, 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
>
Oops, consider this squashed in:
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 496e43dc02..678a434f04 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -3714,6 +3714,7 @@ void virDomainDefFree(virDomainDef *def)
g_free(def->description);
g_free(def->title);
g_free(def->hyperv_vendor_id);
+ g_free(def->tcg_features);
virBlkioDeviceArrayClear(def->blkio.devices,
def->blkio.ndevices);
Michal
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.
> + 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.
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
© 2016 - 2026 Red Hat, Inc.