Re: [PATCH 2/2] qemu: support dirty ring feature

On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81(a)chinatelecom.cn wrote: > From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > > 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&gt; > --- [...] > @@ -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)