On 11/23/21 15:36, huangy81@xxxxxxxxxxxxxxx wrote:
> From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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
