Hi Peter, Peter Krempa, Feb 12, 2025 at 17:17: > 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/0418f90809aea5b375c859e744c8e8610e9be446 > > So what are the plans regarding the managed mode here? > I think I will go first, the way Paolo suggested, deliver the helper thought a RPM, i.e unmanaged by libvirt. I did not find any relevant justification for this simple stateless helper to be managed by libvirt. Technically, the helper could also be distributed as a container. But I guess mixing RPM & container for an install is not very appreciated. At the moment it comes with qemu RPM. The systemd files are included as well. > 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@xxxxxxxxxx> >> --- >> 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. Thanks for all your feedback. I've added all in my todo list for my next sprint. I understand that it needs way more documentation ! Anthony
