[PATCH v7 1/2] qemu: support dirty ring feature
Hyman Huang
huangy81 at chinatelecom.cn
Tue Dec 14 11:20:53 UTC 2021
On 12/14/21 17:22, Michal Prívozník wrote:
> 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
>
Ok, i'll rebase the master once the changes get merged and test if the
dirty ring still works.
--
Best Regards
Hyman Huang(黄勇)
More information about the libvir-list
mailing list