On Fri, Feb 13, 2026 at 10:03:49 +0100, Michal Prívozník wrote: > On 2/12/26 15:21, Peter Krempa wrote: > > On Wed, Feb 11, 2026 at 12:50:24 +0100, Michal Privoznik via Devel wrote: > >> From: Michal Privoznik <[email protected]> > >> > >> Since some hyperv features might be already enabled/disabled when > >> entering qemuProcessEnableDomainFeatures() only those which are > >> not set in domain XML (i.e. are VIR_TRISTATE_SWITCH_ABSENT) > >> should be modified. Furthermore, some features are not a simple > >> on/off switch, but a number or a string even. Well, that doesn't > >> matter really as the logic for setting them is the same: only set > >> their value iff they are not already set. > >> > >> Resolves: https://issues.redhat.com/browse/RHEL-148219 > >> Signed-off-by: Michal Privoznik <[email protected]> > >> --- > >> src/qemu/qemu_process.c | 24 ++++++++++++------- > >> .../hyperv-host-model.x86_64-latest.args | 2 +- > >> 2 files changed, 17 insertions(+), 9 deletions(-) > >> > >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > >> index c5b2a5fda8..27aa1896d4 100644 > >> --- a/src/qemu/qemu_process.c > >> +++ b/src/qemu/qemu_process.c > >> @@ -6960,30 +6960,38 @@ qemuProcessEnableDomainFeatures(virDomainObj *vm) > >> } > >> > >> for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { > >> + virTristateSwitch origState = vm->def->hyperv.features[i]; > >> + > >> if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(hv->features, i)) > >> continue; > >> > >> - vm->def->hyperv.features[i] = VIR_TRISTATE_SWITCH_ON; > >> + if (vm->def->hyperv.features[i] == VIR_TRISTATE_SWITCH_ABSENT) > >> + vm->def->hyperv.features[i] = VIR_TRISTATE_SWITCH_ON; > > > > > > [...] > > > > > >> > >> if (i == VIR_DOMAIN_HYPERV_SPINLOCKS) { > >> if (hv->spinlocks != 0) { > >> - vm->def->hyperv.spinlocks = hv->spinlocks; > >> + if (vm->def->hyperv.spinlocks == 0) > >> + vm->def->hyperv.spinlocks = hv->spinlocks; > > > > > > This feature is parsed as: > > > > > > case VIR_DOMAIN_HYPERV_SPINLOCKS: > > if (value != VIR_TRISTATE_SWITCH_ON) > > break; > > > > if (virXMLPropUIntDefault(node, "retries", 0, VIR_XML_PROP_NONE, > > &def->hyperv.spinlocks, UINT_MAX) < > > 0) { > > return -1; > > } > > > > if (def->hyperv.spinlocks < 0xFFF) { > > virReportError(VIR_ERR_XML_ERROR, "%s", > > _("HyperV spinlock retry count must be at > > least 4095")); > > return -1; > > } > > > > Thus the value will never be 0 if it was enabled in the XML. Practically > > though I don't think it matters because the user in such case set what > > they wanted. > > > > To make it more obvious that this would happen I'd probably skip the > > attempt to set anything if origState of this feature is not _ABSENT. > > > > In fact IMO none of the settings should be auto-filled if the user > > provided config of the feature. > > Ah good point. All I wanted was to fill in other values for particular > enlightenment. > > > > > > >> } else { > >> - vm->def->hyperv.features[i] = VIR_TRISTATE_SWITCH_ABSENT; > >> + vm->def->hyperv.features[i] = origState; > >> } > >> } else if (i == VIR_DOMAIN_HYPERV_STIMER) { > >> - vm->def->hyperv.stimer_direct = hv->stimer_direct; > >> + if (vm->def->hyperv.stimer_direct == > >> VIR_TRISTATE_SWITCH_ABSENT) > >> + vm->def->hyperv.stimer_direct = hv->stimer_direct; > >> /* Both hv-stimer and hv-stimer-direct require hv-time which > >> is > >> * expose as a timer. Enable it. */ > >> qemuProcessMaybeAddHypervTimer(vm->def); > >> } else if (i == VIR_DOMAIN_HYPERV_TLBFLUSH) { > >> - vm->def->hyperv.tlbflush_direct = hv->tlbflush_direct; > >> - vm->def->hyperv.tlbflush_extended = hv->tlbflush_extended; > >> + if (vm->def->hyperv.tlbflush_direct == > >> VIR_TRISTATE_SWITCH_ABSENT) > >> + vm->def->hyperv.tlbflush_direct = hv->tlbflush_direct; > >> + if (vm->def->hyperv.tlbflush_extended == > >> VIR_TRISTATE_SWITCH_ABSENT) > >> + vm->def->hyperv.tlbflush_extended = hv->tlbflush_extended; > >> } else if (i == VIR_DOMAIN_HYPERV_VENDOR_ID) { > >> if (hv->vendor_id) { > >> - vm->def->hyperv.vendor_id = g_strdup(hv->vendor_id); > >> + if (!vm->def->hyperv.vendor_id) > >> + vm->def->hyperv.vendor_id = g_strdup(hv->vendor_id); > > > > e.g. this feature can't exist without having the 'value' field populated > > by the user. > > > > So [1] could then be: > > > > > > if (vm->def->hyperv.features[i] == VIR_TRISTATE_SWITCH_ABSENT) { > > vm->def->hyperv.features[i] = VIR_TRISTATE_SWITCH_ON; > > } else { > > /* if the user provided already config for this we skip the > > * auto-population code */ > > continue; > > } > > > > and the rest would then require no change at all. > > > > Funny, this was my first iteration I had locally. Consider this squashed > in, locally.
Reviewed-by: Peter Krempa <[email protected]>
