[PATCH 0/2] Allow tweaking TCG's tb-size
*** BLURB HERE *** Michal Prívozník (2): conf: Introduce TCG domain features qemu: Generate command line for tb-cache feature docs/formatdomain.rst | 11 +++ docs/schemas/domaincommon.rng | 15 +++- src/conf/domain_conf.c | 90 +++++++++++++++++++ src/conf/domain_conf.h | 7 ++ src/qemu/qemu_command.c | 14 ++- src/qemu/qemu_validate.c | 11 +++ ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++ .../x86_64-default-cpu-tcg-features.xml | 67 ++++++++++++++ tests/qemuxml2argvtest.c | 1 + ...default-cpu-tcg-features.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args 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 -- 2.32.0
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> --- 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 0651975c88..d7f4f093fb 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 67df13d90d..73401e6b75 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6062,7 +6062,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"> @@ -6202,6 +6202,9 @@ <optional> <ref name="ibs"/> </optional> + <optional> + <ref name="tcgfeatures"/> + </optional> </interleave> </element> </optional> @@ -6490,6 +6493,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 4644d18120..2db013cab4 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, @@ -17650,6 +17651,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) @@ -17858,6 +17883,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; } @@ -21526,6 +21556,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) @@ -21676,6 +21739,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; } @@ -27645,6 +27713,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) @@ -27965,6 +28051,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 cb6d8975b8..e2287b3be1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2044,6 +2044,7 @@ typedef enum { VIR_DOMAIN_FEATURE_CFPC, VIR_DOMAIN_FEATURE_SBBC, VIR_DOMAIN_FEATURE_IBS, + VIR_DOMAIN_FEATURE_TCG, VIR_DOMAIN_FEATURE_LAST } virDomainFeature; @@ -2252,6 +2253,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; @@ -2814,6 +2820,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 c4384dbe8b..5ee4d3ffcb 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 a066c35db0..8240982470 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1405,6 +1405,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 Thu, Nov 04, 2021 at 01:22:52PM +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>
Any objection to using underscore (tb_cache), instead of hyphen (tb-cache)? I realize the QEMU option uses the hyphen. The reason I ask is, as you may know, Python does not allow using assignment operators to its variable names. And when OpenStack Nova adds XML modelling classes for <tcg>, "tb-size" will end up being a variable.
</tcg> </features> </domain>
[...] -- /kashyap
On Thu, Nov 04, 2021 at 07:08:53PM +0100, Kashyap Chamarthy wrote:
On Thu, Nov 04, 2021 at 01:22:52PM +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>
Any objection to using underscore (tb_cache), instead of hyphen (tb-cache)? I realize the QEMU option uses the hyphen.
We're not entirely consistent, but use of "-" is quite typical in the features: <kvm> <hidden state='on'/> <hint-dedicated state='on'/> <poll-control state='on'/> <pv-ipi state='off'/> </kvm> So I think it is right as is. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 04, 2021 at 06:16:19PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 04, 2021 at 07:08:53PM +0100, Kashyap Chamarthy wrote:
On Thu, Nov 04, 2021 at 01:22:52PM +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>
Any objection to using underscore (tb_cache), instead of hyphen (tb-cache)? I realize the QEMU option uses the hyphen.
We're not entirely consistent, but use of "-" is quite typical in the features:
<kvm> <hidden state='on'/> <hint-dedicated state='on'/> <poll-control state='on'/> <pv-ipi state='off'/> </kvm>
So I think it is right as is.
Fair enough. I asked becuase I did spot the inconsistencies in the other elements under the <features> element, for other hypervisors: <hyperv> ... <vendor_id state='on' value='KVM Hv'/> </hyperv> ... <xen> <e820_host state='on'/> <passthrough state='on' mode='share_pt'/> </xen> ... -- /kashyap
On Thu, Nov 04, 2021 at 07:09:07PM +0100, Kashyap Chamarthy wrote:
On Thu, Nov 04, 2021 at 01:22:52PM +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>
Any objection to using underscore (tb_cache), instead of hyphen (tb-cache)? I realize the QEMU option uses the hyphen.
The reason I ask is, as you may know, Python does not allow using assignment operators to its variable names. And when OpenStack Nova adds XML modelling classes for <tcg>, "tb-size" will end up being a variable.
I forgot to mention that this is not an issue. It can be trivially solved in Python to handle the hyphen <-> underscore parsing. Sorry for the noise. -- /kashyap
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/229 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++- ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 45278c7108..7d9cb386fd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7031,14 +7031,21 @@ qemuBuildMachineCommandLine(virCommand *cmd, virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM]; virCPUDef *cpu = def->cpu; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + unsigned long long tb_cache = 0; size_t i; + if (def->features[VIR_DOMAIN_FEATURE_TCG] == VIR_TRISTATE_SWITCH_ON) + tb_cache = def->tcg_features->tb_cache; + virCommandAddArg(cmd, "-machine"); virBufferAdd(&buf, def->os.machine, -1); switch ((virDomainVirtType)def->virtType) { case VIR_DOMAIN_VIRT_QEMU: - virBufferAddLit(&buf, ",accel=tcg"); + /* Unfortunately, -machine doesn't accept -accel arguments and the two + * are mutually exclusive. Don't output accel= if we need -accel. */ + if (tb_cache == 0) + virBufferAddLit(&buf, ",accel=tcg"); break; case VIR_DOMAIN_VIRT_KVM: @@ -7288,6 +7295,11 @@ qemuBuildMachineCommandLine(virCommand *cmd, virCommandAddArgBuffer(cmd, &buf); + if (tb_cache > 0) { + virCommandAddArg(cmd, "-accel"); + virCommandAddArgFormat(cmd, "tcg,tb-size=%llu", tb_cache >> 10); + } + return 0; } diff --git a/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args new file mode 100644 index 0000000000..0800ea4294 --- /dev/null +++ b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args @@ -0,0 +1,40 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-guest \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-guest/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-guest/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-guest/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=guest,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-guest/master-key.aes"}' \ +-machine pc-q35-6.2,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg,tb-size=100 \ +-cpu qemu64 \ +-m 4096 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":4294967296}' \ +-overcommit mem-lock=off \ +-smp 4,sockets=4,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device '{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}' \ +-device '{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}' \ +-device '{"driver":"pcie-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"pcie-root-port","port":11,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x3"}' \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.1","addr":"0x0"}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.2","addr":"0x0","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \ +-audiodev id=audio1,driver=none \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.3","addr":"0x0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1c59395203..56ecc46071 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3422,6 +3422,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 Thu, Nov 04, 2021 at 01:22:53PM +0100, Michal Privoznik wrote:
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/229 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++- ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 45278c7108..7d9cb386fd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7031,14 +7031,21 @@ qemuBuildMachineCommandLine(virCommand *cmd, virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM]; virCPUDef *cpu = def->cpu; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + unsigned long long tb_cache = 0; size_t i;
+ if (def->features[VIR_DOMAIN_FEATURE_TCG] == VIR_TRISTATE_SWITCH_ON) + tb_cache = def->tcg_features->tb_cache; + virCommandAddArg(cmd, "-machine"); virBufferAdd(&buf, def->os.machine, -1);
switch ((virDomainVirtType)def->virtType) { case VIR_DOMAIN_VIRT_QEMU: - virBufferAddLit(&buf, ",accel=tcg"); + /* Unfortunately, -machine doesn't accept -accel arguments and the two + * are mutually exclusive. Don't output accel= if we need -accel. */ + if (tb_cache == 0) + virBufferAddLit(&buf, ",accel=tcg");
The "-machine accel=nnn" syntax was the original way to configure accelerator. The "-accel nnn,$opts" syntax was added in QEMU 2.9.0 with commit 8d4e9146b3568022ea5730d92841345d41275d66 Author: KONRAD Frederic <fred.konrad@greensocs.com> Date: Thu Feb 23 18:29:08 2017 +0000 tcg: add options for enabling MTTCG with the accelerator name being sugar for "-machine accel=nnn". Then in 5.0.0, this was reversed so that '-machine accel=nnn' is now sugar for '-accel nnn' commit 6f6e1698a68ceb49e57676528612f22eaf2c16c3 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Wed Nov 13 10:10:47 2019 +0100 vl: configure accelerators from -accel options IOW, long term we want to be using -accel exclusively. Given our minimum QEMU 2.11.0, we can in fact use -accel exclusively already AFAICT. That would simplify this patch so that we're not supporting distinct syntax with tb_cache.
break;
case VIR_DOMAIN_VIRT_KVM: @@ -7288,6 +7295,11 @@ qemuBuildMachineCommandLine(virCommand *cmd,
virCommandAddArgBuffer(cmd, &buf);
+ if (tb_cache > 0) { + virCommandAddArg(cmd, "-accel"); + virCommandAddArgFormat(cmd, "tcg,tb-size=%llu", tb_cache >> 10); + } + return 0; }
diff --git a/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args new file mode 100644 index 0000000000..0800ea4294 --- /dev/null +++ b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args @@ -0,0 +1,40 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-guest \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-guest/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-guest/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-guest/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=guest,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-guest/master-key.aes"}' \ +-machine pc-q35-6.2,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg,tb-size=100 \ +-cpu qemu64 \ +-m 4096 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":4294967296}' \ +-overcommit mem-lock=off \ +-smp 4,sockets=4,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device '{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}' \ +-device '{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}' \ +-device '{"driver":"pcie-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"pcie-root-port","port":11,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x3"}' \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.1","addr":"0x0"}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.2","addr":"0x0","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \ +-audiodev id=audio1,driver=none \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.3","addr":"0x0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1c59395203..56ecc46071 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3422,6 +3422,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
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/4/21 6:25 PM, Daniel P. Berrangé wrote:
On Thu, Nov 04, 2021 at 01:22:53PM +0100, Michal Privoznik wrote:
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/229 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++- ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args
IOW, long term we want to be using -accel exclusively.
Given our minimum QEMU 2.11.0, we can in fact use -accel exclusively already AFAICT.
That would simplify this patch so that we're not supporting distinct syntax with tb_cache.
Ah, I don't know why I thought the opposite. Let me post v2. Michal
On Fri, Nov 05, 2021 at 09:28:16AM +0100, Michal Prívozník wrote:
On 11/4/21 6:25 PM, Daniel P. Berrangé wrote:
On Thu, Nov 04, 2021 at 01:22:53PM +0100, Michal Privoznik wrote:
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/229 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++- ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args
IOW, long term we want to be using -accel exclusively.
Given our minimum QEMU 2.11.0, we can in fact use -accel exclusively already AFAICT.
That would simplify this patch so that we're not supporting distinct syntax with tb_cache.
Ah, I don't know why I thought the opposite. Let me post v2.
It is easily misunderstood, because QEMU does not makes it at all clear which syntax is preferred, and its further confused by the fact that they changed their mind & reversed the syntax sugaring :-) Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 04, 2021 at 01:22:53PM +0100, Michal Privoznik wrote:
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start.
Hmm, you write: "... we can use the old -accel", I was told by Paolo Bonzini the other day that "-accel" is preferred instead of "-machine accel". Hence I filed this GitLab ticket a couple of days: https://gitlab.com/libvirt/libvirt/-/issues/233 (Use `-accel` instead of `-machine accel`) [...] -- /kashyap
On 11/4/21 6:51 PM, Kashyap Chamarthy wrote:
On Thu, Nov 04, 2021 at 01:22:53PM +0100, Michal Privoznik wrote:
Alright, here's the deal: to enable tb-cache one has to use '-accel tcg,tb-size=' which then conflicts with '-machine accel=tcg'. But sure, we can use the old -accel in this specific case. But because of how the tb-size argument is defined in QEMU there's no way for us to have a capability check. The feature was introduced in QEMU commit of v5.0.0-rc0~175^2~62 and is tied to TCG only. Therefore, I think we can live without capability check. Worst case scenario, QEMU fails to start.
Hmm, you write: "... we can use the old -accel", I was told by Paolo Bonzini the other day that "-accel" is preferred instead of "-machine accel". Hence I filed this GitLab ticket a couple of days:
https://gitlab.com/libvirt/libvirt/-/issues/233 (Use `-accel` instead of `-machine accel`)
[...]
Yeah, I don't know why I thought that -accel was older than -machine accel. Silly me :-) let me post v2. Michal
On Thu, Nov 04, 2021 at 01:22:51PM +0100, Michal Privoznik wrote: [...]
Michal Prívozník (2): conf: Introduce TCG domain features qemu: Generate command line for tb-cache feature
docs/formatdomain.rst | 11 +++ docs/schemas/domaincommon.rng | 15 +++- src/conf/domain_conf.c | 90 +++++++++++++++++++ src/conf/domain_conf.h | 7 ++ src/qemu/qemu_command.c | 14 ++- src/qemu/qemu_validate.c | 11 +++ ...efault-cpu-tcg-features.x86_64-latest.args | 40 +++++++++ .../x86_64-default-cpu-tcg-features.xml | 67 ++++++++++++++ tests/qemuxml2argvtest.c | 1 + ...default-cpu-tcg-features.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.x86_64-latest.args 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
Thanks! Works for me. Tested-by: Kashyap Chamarthy <kchamart@redhat.com> I built your patches on top of: $> git describe v7.9.0-55-g20e64dad07 And with "tb-size" 64MiB (65536 KiB), quoting only partial libvirt XML): <domain type='qemu' id='4'> <name>cvm2</name> <uuid>835c272a-ba89-4227-b389-36f7b9d24345</uuid> <memory unit='KiB'>2097152</memory> <currentMemory unit='KiB'>2097152</currentMemory> <vcpu placement='static'>1</vcpu> <resource> <partition>/machine</partition> </resource> <os> <type arch='x86_64' machine='pc-i440fx-5.0'>hvm</type> <loader readonly='yes' type='pflash'>/usr/share/edk2/ovmf/OVMF_CODE.fd</loader> <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/cvm2_VARS.fd</nvram> <boot dev='hd'/> </os> <features> <acpi/> <apic/> <tcg> <tb-cache unit='KiB'>65536</tb-cache> </tcg> </features> <cpu mode='custom' match='exact' check='full'> <model fallback='forbid'>Nehalem</model> <feature policy='require' name='hypervisor'/> </cpu> [...] </domain> It generates the right QEMU command-line (also partial): /bin/qemu-kvm \ -name guest=cvm2,debug-threads=on \ -S \ -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain-4-cvm2/master-key.aes"}' \ -blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \ -blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/cvm2_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ -machine pc-i440fx-5.0,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ -accel tcg,tb-size=64 \ -cpu Nehalem \ -m 2048 \ -object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":2147483648}' \ [...] -- /kashyap
participants (4)
-
Daniel P. Berrangé -
Kashyap Chamarthy -
Michal Privoznik -
Michal Prívozník