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.
Michal
