2:29 a.m.
On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81(a)chinatelecom.cn wrote:
From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn>
QEMU has introduced a dirty ring feature, this patch add corresponding
feature named 'dirty-ring', which enable dirty ring feature when
starting vm.
To enable the feature, libvirt add "-accel dirty-ring-size=xxx"
to QEMU command line, the following XML needs to be added to
the guest's domain description:
<features>
<kvm>
<dirty-ring state='on' size='xxx'>
</kvm>
</features>
If property "state=on" but property "size" not be configured, set
default ring size with 4096.
Since dirty ring can only be enabled by specifying "-accel" option
and do not support the legacy style, it seems that there's no
other way to work around this, so we use "-accel" option to specify
accelerator instead of "-machine" when building qemu commandline.
Details about the qemu "-accel" option:
https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@r...
Three things this patch has done:
Note we require that patches are kept small and self contained. You
describe 3 things this patch is doing ant thus it almost implies as
it could be split 3-ways ...
1. switch the option "-machine accel" to "-accel"
when specifying
accelerator type once libvirt build QEMU command line.
this definitely belongs to a separate patch.
2. Since this patch change the output of building commandline, qemuxml2argvtest
should also been modified so that test cases can be executed
successfully, we modify test cases involved from qemu-2.9.0 to
Note that the oldest supported qemu is now qemu-2.11
qemu-6.1.0
3. Introduce dirty_ring_size in struct "_virDomainDef" to hold the
ring size configured by user, and pass dirty_ring_size when building qemu
commandline if dirty ring feature enabled.
This requires at least a 2 way split.
1) patch adding the definition, docs and XML parser/formatter
2) actual qemu impl.
Signed-off-by: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn>
---
[...]
@@ -17574,6 +17575,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) {
int feature;
virTristateSwitch value;
+ xmlNodePtr node = NULL;
+
+ node = ctxt->node;
if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) <
0)
return -1;
@@ -17590,12 +17594,40 @@ virDomainFeaturesDefParse(virDomainDef *def,
case VIR_DOMAIN_KVM_HIDDEN:
case VIR_DOMAIN_KVM_DEDICATED:
case VIR_DOMAIN_KVM_POLLCONTROL:
+ case VIR_DOMAIN_KVM_DIRTY_RING:
Don't integrate this with other cases ...
if (virXMLPropTristateSwitch(nodes[i],
"state",
VIR_XML_PROP_REQUIRED,
&value) < 0)
return -1;
Preferably rather duplicate this parsing ....
def->kvm_features[feature] = value;
+
+ /* only for dirty ring case */
+ if (((virDomainKVM) feature) == VIR_DOMAIN_KVM_DIRTY_RING
&&
+ value == VIR_TRISTATE_SWITCH_ON) {
To avoid this condition.
+ ctxt->node = nodes[i];
+
+ if (virXMLPropString(ctxt->node, "size")) {
+ if (virXPathUInt("string(./@size)", ctxt,
+ &def->dirty_ring_size) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid number of dirty ring
size"));
+ return -1;
+ }
+
+ if ((def->dirty_ring_size & (def->dirty_ring_size
- 1)) ||
Zero checks on integers should be explicit.
+ def->dirty_ring_size < 1024
||
+ def->dirty_ring_size > 65536) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("dirty ring must be power of 2
"
+ "and ranges [1024,
65536]"));
+ return -1;
+ }
+ } else {
+ /* ring size not configured, set with default value 4096 */
+ def->dirty_ring_size = 4096;
Assigning defaults in the parser isn't current state of things.
Preferably this belongs to a post-parse callback.
+ }
+ }
break;
case VIR_DOMAIN_KVM_LAST:
[...]
@@ -21627,6 +21660,7 @@
virDomainDefFeaturesCheckABIStability(virDomainDef *src,
case VIR_DOMAIN_KVM_HIDDEN:
case VIR_DOMAIN_KVM_DEDICATED:
case VIR_DOMAIN_KVM_POLLCONTROL:
+ case VIR_DOMAIN_KVM_DIRTY_RING:
if (src->kvm_features[i] != dst->kvm_features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of KVM feature '%s' differs:
"
The abi-stability check should also check the size.
@@ -27614,6 +27648,15 @@ virDomainDefFormatFeatures(virBuffer *buf,
def->kvm_features[j]));
break;
+ case VIR_DOMAIN_KVM_DIRTY_RING:
+ if (def->kvm_features[j])
Make the comparison with VIR_TRISTATE_SWITCH_ABSENT explicit here.
+ virBufferAsprintf(&childBuf,
"<%s state='%s' size='%d'/>\n",
+ virDomainKVMTypeToString(j),
+ virTristateSwitchTypeToString(
+ def->kvm_features[j]),
Don't linebreak the argument to virTristateSwitchTypeToString even if it
exceeds the line lenght. It's unreadable this way.
+ def->dirty_ring_size);
+ break;
+
case VIR_DOMAIN_KVM_LAST:
break;
}
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ea51369..80142b2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6587,6 +6587,9 @@ qemuBuildCpuCommandLine(virCommand *cmd,
virBufferAddLit(&buf, ",kvm-poll-control=on");
break;
+ case VIR_DOMAIN_KVM_DIRTY_RING:
+ break;
+
case VIR_DOMAIN_KVM_LAST:
break;
}
@@ -6758,29 +6761,41 @@ qemuBuildNameCommandLine(virCommand *cmd,
return 0;
}
+static void
+qemuBuildAccelCommandLineTcgOptions(void)
+{
+ /* TODO: build command line tcg accelerator
+ * property like tb-size */
Is this patch incomplete? In case you don't want to support this for TCG
you should forbid it in validation rather than silently ignoring it.
+}
+
+static void
+qemuBuildAccelCommandLineKvmOptions(virBuffer *buf,
+ const virDomainDef *def)
+{
+ if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON &&
+ def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) {
+ virBufferAsprintf(buf, ",dirty-ring-size=%d",
def->dirty_ring_size);
+ }
+}
+
static int
-qemuBuildMachineCommandLine(virCommand *cmd,
- virQEMUDriverConfig *cfg,
- const virDomainDef *def,
- virQEMUCaps *qemuCaps,
- qemuDomainObjPrivate *priv)
+qemuBuildAccelCommandLine(virCommand *cmd,
+ const virDomainDef *def)
{
- virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
- virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
- virCPUDef *cpu = def->cpu;
+ /* the '-machine' options for accelerator are legacy,
+ * using the '-accel' options by default */
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- size_t i;
-
- virCommandAddArg(cmd, "-machine");
- virBufferAdd(&buf, def->os.machine, -1);
+ virCommandAddArg(cmd, "-accel");
This MUST be a separate commit it's a fully separate change which
requires separate justification.
switch ((virDomainVirtType)def->virtType) {
case VIR_DOMAIN_VIRT_QEMU:
- virBufferAddLit(&buf, ",accel=tcg");
+ virBufferAddLit(&buf, "tcg");
+ qemuBuildAccelCommandLineTcgOptions();
break;
case VIR_DOMAIN_VIRT_KVM:
- virBufferAddLit(&buf, ",accel=kvm");
+ virBufferAddLit(&buf, "kvm");
+ qemuBuildAccelCommandLineKvmOptions(&buf, def);
break;
case VIR_DOMAIN_VIRT_KQEMU:
@@ -6808,6 +6823,61 @@ qemuBuildMachineCommandLine(virCommand *cmd,
return -1;
}
+ virCommandAddArgBuffer(cmd, &buf);
+ return 0;
+}
+
+static int
+qemuBuildMachineCommandLine(virCommand *cmd,
+ virQEMUDriverConfig *cfg,
+ const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ qemuDomainObjPrivate *priv)
+{
+ virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
+ virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
+ virCPUDef *cpu = def->cpu;
+ g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+ size_t i;
+
+ virCommandAddArg(cmd, "-machine");
+ virBufferAdd(&buf, def->os.machine, -1);
+
+ /* QEMU lower than 2.9.0 do not support '-accel'
+ * fallback to '-machine accel' */
We don't support those qemu versions. This is the reason why you must
split the patch. A refactor like this has nothing to do with the feature
addition and should be justified and reviewed separately.
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
+ switch ((virDomainVirtType)def->virtType) {
+ case VIR_DOMAIN_VIRT_QEMU:
+ virBufferAddLit(&buf, ",accel=tcg");
+ break;
+ case VIR_DOMAIN_VIRT_KVM:
+ virBufferAddLit(&buf, ",accel=kvm");
+ break;
+ case VIR_DOMAIN_VIRT_KQEMU:
[...]
@@ -10402,6 +10472,11 @@ qemuBuildCommandLine(virQEMUDriver *driver,
if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps, priv) < 0)
return NULL;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
+ if (qemuBuildAccelCommandLine(cmd, def) < 0)
+ return NULL;
+ }
+
qemuBuildTSEGCommandLine(cmd, def);
if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)