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

On 11/23/21 15:36, huangy81(a)chinatelecom.cn wrote: > From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > > 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(a)chinatelecom.cn&gt; > --- > 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