Re: [PATCH v3 1/1] qemu: Add support for RAPL MSRs feature

On Wed, Feb 12, 2025 at 16:36:48 +0100, Anthony Harivel wrote: > Add the support in libvirt to activate the RAPL feature in QEMU. > > This feature is activated with -accel kvm,rapl=true,path=/path/sock.sock > in QEMU. > > The feature is activated in libvirt with the following XML > configuration: > > <kvm> > [...] > <rapl state ='on' socket='/run/qemu-vmsr-helper.sock'/> > [...] > </kvm> > > See: https://gitlab.com/qemu-project/qemu/-/commit/0418f90809aea5b375c859e744c... So what are the plans regarding the managed mode here?

I don't necessarily need you to commit to anything but if libvirt is ever expected to support the managed mode we should at least have a strategy how to integrate that and document that only non-managed mode is supported. E.g. prepare for <rapl state ='on' mode='unmanaged' socket='/run/qemu-vmsr-helper.sock'/> Or at least imply in the documentation that if socket is provided we expect an manually managed daemon to listen there. > Signed-off-by: Anthony Harivel <aharivel(a)redhat.com&gt; > --- > docs/formatdomain.rst | 2 ++ > src/conf/domain_conf.c | 18 ++++++++++++++++++ > src/conf/domain_conf.h | 2 ++ > src/conf/schemas/domaincommon.rng | 10 ++++++++++ > src/qemu/qemu_command.c | 12 ++++++++++++ > tests/qemuxmlconfdata/kvm-features-off.xml | 1 + > .../kvm-features.x86_64-latest.args | 2 +- > tests/qemuxmlconfdata/kvm-features.xml | 1 + > 8 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index 83aeaa32c296..eceee1531085 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -2016,6 +2016,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. > <poll-control state='on'/> > <pv-ipi state='off'/> > <dirty-ring state='on' size='4096'/> > + <rapl state ='on' socket='/run/qemu-vmsr-helper.sock'/> > </kvm> > <xen> > <e820_host state='on'/> > @@ -2135,6 +2136,7 @@ are: > 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:`8.0.0 (QEMU 6.1)` > + rapl Enable rapl feature on, off; socket - rapl helper socket :since:`10.8.0 (QEMU 9.1)` > ============== ============================================================================ ====================================================== ============================ The libvirt version here will need updating. Since this works only on intel: $ qemu-system-x86_64 --accel kvm,rapl=true qemu-system-x86_64: --accel kvm,rapl=true: The RAPL feature can only be enabled on hosts with Intel CPU models qemu-system-x86_64: --accel kvm,rapl=true: kvm : error RAPL feature requirement not met you should document that. I don't think we need a specific CPU check for this niche feature. > @@ -16837,6 +16838,11 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, > return -1; > } > } > + > + /* rapl feature should parse socket property */ > + if (feature == VIR_DOMAIN_KVM_RAPL && value == VIR_TRISTATE_SWITCH_ON) { > + def->kvm_features->rapl_helper_socket = virXMLPropString(feat, "socket"); > + } > } Qemu will fail without a socket. $ qemu-system-x86_64 --accel kvm,rapl=true qemu-system-x86_64: --accel kvm,rapl=true: vmsr socket opening failed If you make the socket mandatory the presence of the socket could later be used to mean un-managed mode. > > def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON; > @@ -21308,6 +21314,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, > case VIR_DOMAIN_KVM_POLLCONTROL: > case VIR_DOMAIN_KVM_PVIPI: > case VIR_DOMAIN_KVM_DIRTY_RING: > + case VIR_DOMAIN_KVM_RAPL: > if (src->kvm_features->features[i] != dst->kvm_features->features[i]) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("State of KVM feature '%1$s' differs: source: '%2$s', destination: '%3$s'"), > @@ -28107,6 +28114,17 @@ virDomainDefFormatFeatures(virBuffer *buf, [...] > typedef struct _virDomainFeatureTCG virDomainFeatureTCG; > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng > index 96cedb85e867..c910df8c75a0 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -8143,6 +8143,16 @@ > </optional> > </element> > </optional> > + <optional> > + <element name="rapl"> > + <ref name="featurestate"/> > + <optional> > + <attribute name="socket"> > + <ref name="absFilePath"/> > + </attribute> > + </optional> > + </element> > + </optional> > </interleave> > </element> > </define> [...] > @@ -7062,6 +7065,15 @@ qemuBuildAccelCommandLine(virCommand *cmd, > def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) { > virBufferAsprintf(&buf, ",dirty-ring-size=%d", def->kvm_features->dirty_ring_size); > } > + > + if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON && > + def->kvm_features->features[VIR_DOMAIN_KVM_RAPL] == VIR_TRISTATE_SWITCH_ON) { > + virBufferAddLit(&buf, ",rapl=true"); > + > + if (def->kvm_features->rapl_helper_socket != NULL) { > + virBufferAsprintf(&buf, ",rapl-helper-socket=%s", def->kvm_features->rapl_helper_socket); > + } As noted above; qemu makes the socket mandatory so the check doesn't make much sense. Also we'll most likely need to use virQEMUBuildBufferEscapeComma here to properly format paths with a comma since they are user-provided. > + } > break; > > case VIR_DOMAIN_VIRT_HVF: The code doesn't also do any security labelling of the socket. That means that the socket needs to be properly labelled manually. I think that should be documetned as well, so that there's no expectation of libvirt to do this.