在 2021/6/21 15:29, Peter Krempa 写道:
On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81@xxxxxxxxxxxxxxx wrote:
From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
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@xxxxxxxxxx/
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
ok, i'll fix it.
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.
ok, i'll split it next version.
Signed-off-by: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
---
[...]
@@ -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.
get it.
+ }
+ }
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.
indeed, seems weird, i'll fix it
+ 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.
ok, i'll forbid it next version.
+}
+
+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.
ok.
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.
ok. i'll post a patch just changing the way of build commandline before
the dirty ring feature patchset.
+ 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)
