Re: [PATCH 00/10] KVM: Avoid literal numbers as return values

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3507 bytes --]

On 05.12.25 15:16, Sean Christopherson wrote:
> On Fri, Dec 05, 2025, Juergen Gross wrote:
>> This series is the first part of replacing the use of literal numbers
>> (0 and 1) as return values with either true/false or with defines.
> 
> Sorry, but NAK to using true/false.  IMO, it's far worse than 0/1.  At least 0/1
> draws from the kernel's 0/-errno approach.  With booleans, the polarity is often
> hard to discern without a priori knowledge of the pattern, and even then it can
> be confusing.  E.g. for me, returning "true" when .set_{c,d}r() fails is unexpected,
> and results in unintuitive code like this:
> 
>                  if (!kvm_dr6_valid(val))
> 			return true;

I don't see "return 1;" being much better here.

> For isolated APIs whose values aren't intented to be propagated back up to the
> .handle_exit() call site, I would much rather return 0/-EINVAL.

Fine with me (I agree this would be more readable).

> Do you have a sketch of what the end goal/result will look like?  IIRC, last time
> anyone looked at doing this (which was a few years ago, but I don't think KVM has
> changed _that_ much), we backed off because a partial conversion would leave KVM
> in an unwieldy and somewhat scary state.

In the end I'd like to get rid of most "return 1;" and several "return 0;"
instances in KVM.

The main reason is that it is sometimes very hard to determine what the
current "return 1" is meant to say ("error" or "return to guest" or just
"okay"). This is especially true in some of the low level MSR emulation
code, e.g. in kvm_pmu_get_msr(): only after examining the call paths I was
sure the "return 0" wasn't meant to return to qemu, but to indicate success.

I have already started to replace the "return 1;" instances in the exit
handlers with "return KVM_RET_GUEST;", but the MSR emulation code convinced
me to analyze it first and to clear it up before changing any of its "1"
return values by accident to "KVM_RET_GUEST".

In the end my plan is to cover all archs to replace the literal "1"s with
"KVM_RET_GUEST" where appropriate, and as many other literal "1"s as possible
with more meaningful defines.

I hoped to get this done much earlier and faster, but this is quite a yak to
shave. :-)

I realized that pushing out patches as soon as possible is the only way to
get this finished at all, as this is a moving target with all the work of
others which might interfere. So my revised plan is to do one arch after
the other and in each arch to cover stuff like the MSR emulation first in
order not to mix things up again.

>> This work is a prelude of getting rid of the magic value "1" for
>> "return to guest". I started in x86 KVM host code doing that and soon
>> stumbled over lots of other use cases of the magic "1" as return value,
>> especially in MSR emulation where a comment even implied this "1" was
>> due to the "return to guest" semantics.
>>
>> A detailed analysis of all related code paths revealed that there was
>> indeed a rather clean interface between the functions using the MSR
>> emulation "1" and those using the "return to guest" "1".
> 
> Ya, we've started chipping away at the MSR stuff.  The big challenge is avoiding
> subtle ABI changes related to the fixups done by kvm_do_msr_access().

Right.

This whole work was triggered by my accidental "fix" of kvm_mmu_page_fault()
replacing a "1" with "RET_PF_RETRY", which you stopped from hitting upstream.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]