[PATCH v7 1/2] qemu: support dirty ring feature
Michal Prívozník
mprivozn at redhat.com
Tue Dec 14 09:22:17 UTC 2021
On 11/23/21 15:36, huangy81 at chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
>
> Dirty ring feature was introduced in qemu-6.1.0, this patch
> add the corresponding feature named 'dirty-ring', which enable
> dirty ring feature when starting vm.
>
> To implement the dirty-ring feature, dirty_ring_size in struct
> "_virDomainDef" is introduced to hold the dirty ring size
> configured in xml, and it will be used as dirty-ring-size
> property of kvm accelerator when building qemu commandline,
> it is something like "-accel dirty-ring-size=xxx".
>
> To enable the feature, 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", property "size" must be specified, which
> should be power of 2 and range in [1024, 65526].
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
> ---
> docs/formatdomain.rst | 18 ++++++------
> docs/schemas/domaincommon.rng | 10 +++++++
> src/conf/domain_conf.c | 54 +++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 4 +++
> src/qemu/qemu_command.c | 12 ++++++++
> 5 files changed, 90 insertions(+), 8 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index eb8c973cf1..ea69b61c70 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -1843,6 +1843,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
> <hint-dedicated state='on'/>
> <poll-control state='on'/>
> <pv-ipi state='off'/>
> + <dirty-ring state='on' size='4096'/>
I was confused at first, what units is @size in but looking into the
qemu docs it's unit-less number:
"[dirty-ring-size] it controls the size of the per-vCPU dirty page
ring buffer (number of entries for each vCPU)."
Therefore I'm okay with having it as a plain attribute. Otherwise for
values with units (traditionally size units like KiB/MiB/...) I would
advise to go with an extra element.
> </kvm>
> <xen>
> <e820_host state='on'/>
> @@ -1925,14 +1926,15 @@ are:
> ``kvm``
> Various features to change the behavior of the KVM hypervisor.
>
> - ============== ============================================================================ ======= ============================
> - Feature Description Value Since
> - ============== ============================================================================ ======= ============================
> - hidden Hide the KVM hypervisor from standard MSR based discovery on, off :since:`1.2.8 (QEMU 2.1.0)`
> - hint-dedicated Allows a guest to enable optimizations when running on dedicated vCPUs on, off :since:`5.7.0 (QEMU 2.12.0)`
> - poll-control Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)`
> - pv-ipi Paravirtualized send IPIs on, off :since:`7.10.0 (QEMU 3.1)`
> - ============== ============================================================================ ======= ============================
> + ============== ============================================================================ ====================================================== ============================
> + Feature Description Value Since
> + ============== ============================================================================ ====================================================== ============================
> + hidden Hide the KVM hypervisor from standard MSR based discovery on, off :since:`1.2.8 (QEMU 2.1.0)`
> + hint-dedicated Allows a guest to enable optimizations when running on dedicated vCPUs on, off :since:`5.7.0 (QEMU 2.12.0)`
> + poll-control Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)`
> + pv-ipi Paravirtualized send IPIs on, off :since:`7.10.0 (QEMU 3.1)`
> + dirty-ring Enable dirty ring feature on, off; size - must be power of 2, range [1024,65536] :since:`7.10.0 (QEMU 6.1)`
> + ============== ============================================================================ ====================================================== ============================
>
> ``xen``
> Various features to change the behavior of the Xen hypervisor.
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f01b7a6470..5f9fe3cc58 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -7212,6 +7212,16 @@
> <ref name="featurestate"/>
> </element>
> </optional>
> + <optional>
> + <element name="dirty-ring">
> + <ref name="featurestate"/>
> + <optional>
> + <attribute name="size">
> + <data type="unsignedInt"/>
> + </attribute>
> + </optional>
> + </element>
> + </optional>
> </interleave>
> </element>
> </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 552d43b845..6f3c925b55 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -205,6 +205,7 @@ VIR_ENUM_IMPL(virDomainKVM,
> "hint-dedicated",
> "poll-control",
> "pv-ipi",
> + "dirty-ring",
> );
>
> VIR_ENUM_IMPL(virDomainXen,
> @@ -17589,6 +17590,25 @@ virDomainFeaturesKVMDefParse(virDomainDef *def,
>
> def->kvm_features[feature] = value;
>
> + /* dirty ring feature should parse size property */
> + if (((virDomainKVM) feature == VIR_DOMAIN_KVM_DIRTY_RING) &&
> + (value == VIR_TRISTATE_SWITCH_ON)) {
> +
> + if (virXMLPropUInt(node, "size", 0, VIR_XML_PROP_REQUIRED,
> + &def->dirty_ring_size) < 0) {
> + return -1;
> + }
> +
> + if ((def->dirty_ring_size != VIR_ROUND_UP_POWER_OF_TWO(def->dirty_ring_size)) ||
VIR_IS_POW2() which works even for other types than uint.
> + 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;
> + }
> + }
> +
> node = xmlNextElementSibling(node);
> }
>
> @@ -21824,7 +21844,27 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
> virTristateSwitchTypeToString(dst->kvm_features[i]));
> return false;
> }
> + break;
>
> + 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: "
> + "source: '%s', destination: '%s'"),
> + virDomainKVMTypeToString(i),
> + virTristateSwitchTypeToString(src->kvm_features[i]),
> + virTristateSwitchTypeToString(dst->kvm_features[i]));
> + return false;
> + }
> +
> + if (src->dirty_ring_size != dst->dirty_ring_size) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("dirty ring size of KVM feature '%s' differs: "
> + "source: '%d', destination: '%d'"),
> + virDomainKVMTypeToString(i),
> + src->dirty_ring_size, dst->dirty_ring_size);
> + return false;
> + }
> break;
>
> case VIR_DOMAIN_KVM_LAST:
> @@ -27872,6 +27912,20 @@ virDomainDefFormatFeatures(virBuffer *buf,
> def->kvm_features[j]));
> break;
>
> + case VIR_DOMAIN_KVM_DIRTY_RING:
> + if (def->kvm_features[j] != VIR_TRISTATE_SWITCH_ABSENT) {
> + virBufferAsprintf(&childBuf, "<%s state='%s'",
> + virDomainKVMTypeToString(j),
> + virTristateSwitchTypeToString(def->kvm_features[j]));
> + if (def->dirty_ring_size) {
> + virBufferAsprintf(&childBuf, " size='%d'/>\n",
> + def->dirty_ring_size);
> + } else {
> + virBufferAddLit(&childBuf, "/>\n");
> + }
> + }
> + break;
> +
> case VIR_DOMAIN_KVM_LAST:
> break;
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8634960313..026edde88f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2084,6 +2084,7 @@ typedef enum {
> VIR_DOMAIN_KVM_DEDICATED,
> VIR_DOMAIN_KVM_POLLCONTROL,
> VIR_DOMAIN_KVM_PVIPI,
> + VIR_DOMAIN_KVM_DIRTY_RING,
>
> VIR_DOMAIN_KVM_LAST
> } virDomainKVM;
> @@ -2933,6 +2934,9 @@ struct _virDomainDef {
> should be re-run before starting */
>
> unsigned int scsiBusMaxUnit;
> +
> + /* size of dirty ring for each vcpu */
> + unsigned int dirty_ring_size;
This feels weird. I haven't followed previous versions that closely, but
what I did for TCG features is introducing a struct that lives close to
other features. This could then be something like:
typedef struct _virDomainFeatureKVM virDomainFeatureKVM;
struct _virDomainFeatureKVM {
int features[VIR_DOMAIN_KVM_LAST];
unsigned int dirty_ring_size; /* size of dirty ring for each vCPU, no units */
};
struct _virDomainDef {
...
- int kvm_features[VIR_DOMAIN_KVM_LAST];
+ virDomainFeatureKVM *kvm_features;
...
};
So here's what I suggest doing - let me post a patch that changes 'int
kvm_features' into a separate struct. I would squash it into yours but
it turned out to be quite lengthy change. Then I'll do changes necessary
for your patch (which will be trivial after that).
Michal
More information about the libvir-list
mailing list