[PATCH 2/2] qemu: support dirty ring feature
Peter Krempa
pkrempa at redhat.com
Mon Jun 21 07:29:02 UTC 2021
On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81 at chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81 at 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@redhat.com/
>
> 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 at 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)
More information about the libvir-list
mailing list