Using a literal 1 instead of RET_PF_RETRY is not nice, fix that.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kvm/mmu/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 8e853a5fc867..d4a9f845b373 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6157,7 +6157,7 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 		vcpu->stat.pf_spurious++;
 
 	if (r != RET_PF_EMULATE)
-		return 1;
+		return RET_PF_RETRY;
 
 emulate:
 	return x86_emulate_instruction(vcpu, cr2_or_gpa, emulation_type, insn,
-- 
2.43.0

Queued, thanks.

Paolo

On Fri, Nov 08, 2024, Paolo Bonzini wrote:
> Queued, thanks.

Noooo!  Can you un-queue?

The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/-errno.
I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.

On 11/8/24 19:44, Sean Christopherson wrote:
> On Fri, Nov 08, 2024, Paolo Bonzini wrote:
>> Queued, thanks.
> 
> Noooo!  Can you un-queue?

Yes, I hadn't even pushed it to kvm/queue.  I applied it out of a whim 
but then realized that it wasn't really -rc7 material.

> The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/-errno.
> I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
> E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.

Gah, I even checked the function and was messed up by the other "return 
RET_PF_RETRY".

If you add X86EMUL_* to the mix, it's even worse.  I had to read this 
three times to understand that it was *not* returning X86EMUL_CONTINUE 
by mistake.  Can I haz strongly-typed enums like in C++?...

         r = kvm_check_emulate_insn(vcpu, emulation_type, insn, insn_len);
         if (r != X86EMUL_CONTINUE) {
		...
         }

         if (!(emulation_type & EMULTYPE_NO_DECODE)) {
                 kvm_clear_exception_queue(vcpu);
                 if (kvm_vcpu_check_code_breakpoint(vcpu, 
emulation_type, &r))
                         return r;
		...
	}

So yeah this really has to be fixed the right way, after all even 
RET_PF_* started out as a conversion from 0/1.

Obligatory bikeshedding, how do KVM_RET_USER and KVM_RET_GUEST sound like?

Paolo

On 09.11.24 09:03, Paolo Bonzini wrote:
> On 11/8/24 19:44, Sean Christopherson wrote:
>> On Fri, Nov 08, 2024, Paolo Bonzini wrote:
>>> Queued, thanks.
>>
>> Noooo!  Can you un-queue?
> 
> Yes, I hadn't even pushed it to kvm/queue.  I applied it out of a whim but then 
> realized that it wasn't really -rc7 material.
> 
>> The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/- 
>> errno.
>> I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
>> E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.
> 
> Gah, I even checked the function and was messed up by the other "return 
> RET_PF_RETRY".
> 
> If you add X86EMUL_* to the mix, it's even worse.  I had to read this three 
> times to understand that it was *not* returning X86EMUL_CONTINUE by mistake.  
> Can I haz strongly-typed enums like in C++?...
> 
>          r = kvm_check_emulate_insn(vcpu, emulation_type, insn, insn_len);
>          if (r != X86EMUL_CONTINUE) {
>          ...
>          }
> 
>          if (!(emulation_type & EMULTYPE_NO_DECODE)) {
>                  kvm_clear_exception_queue(vcpu);
>                  if (kvm_vcpu_check_code_breakpoint(vcpu, emulation_type, &r))
>                          return r;
>          ...
>      }
> 
> So yeah this really has to be fixed the right way, after all even RET_PF_* 
> started out as a conversion from 0/1.
> 
> Obligatory bikeshedding, how do KVM_RET_USER and KVM_RET_GUEST sound like?

+1


Juergen

On 08.11.24 19:44, Sean Christopherson wrote:
> On Fri, Nov 08, 2024, Paolo Bonzini wrote:
>> Queued, thanks.
> 
> Noooo!  Can you un-queue?
> 
> The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/-errno.
> I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
> E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.

And what about the existing "return RET_PF_RETRY" further up?


Juergen

On Fri, Nov 08, 2024, Jürgen Groß wrote:
> On 08.11.24 19:44, Sean Christopherson wrote:
> > On Fri, Nov 08, 2024, Paolo Bonzini wrote:
> > > Queued, thanks.
> > 
> > Noooo!  Can you un-queue?
> > 
> > The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/-errno.
> > I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
> > E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.
> 
> And what about the existing "return RET_PF_RETRY" further up?

Oof.  Works by coincidence.  The intent in that case is to retry the fault, but
the fact that RET_PF_RETRY happens to be '1' is mostly luck.  Returning a postive
value other than '1' should work, but as called out by the comments for the enum,
using '0' for CONTINUE isn't a hard requirement.  E.g. if for some reason we used
'0' for RET_PF_RETRY, this code would break.

 * Note, all values must be greater than or equal to zero so as not to encroach
 * on -errno return values.  Somewhat arbitrarily use '0' for CONTINUE, which
 * will allow for efficient machine code when checking for CONTINUE, e.g.
 * "TEST %rax, %rax, JNZ", as all "stop!" values are non-zero.

FWIW, you are far from the first person to complain about KVM's mostly-undocumented
0/1/-errno return encoding scheme.  The problems is that it's so pervasive
throughout KVM, that in some cases it's not easy to understand if a function is
actually using that scheme, or just happens to return similar values.  I.e.
converting to enums (or #defines) would require a lot of work and churn.

On 08.11.24 23:12, Sean Christopherson wrote:
> On Fri, Nov 08, 2024, Jürgen Groß wrote:
>> On 08.11.24 19:44, Sean Christopherson wrote:
>>> On Fri, Nov 08, 2024, Paolo Bonzini wrote:
>>>> Queued, thanks.
>>>
>>> Noooo!  Can you un-queue?
>>>
>>> The return from kvm_mmu_page_fault() is NOT RET_PF_xxx, it's KVM outer 0/1/-errno.
>>> I.e. '1' is saying "resume the guest", it has *nothing* to do with RET_PF_RETRY.
>>> E.g. that path also handles RET_PF_FIXED, RET_PF_SPURIOUS, etc.
>>
>> And what about the existing "return RET_PF_RETRY" further up?
> 
> Oof.  Works by coincidence.  The intent in that case is to retry the fault, but
> the fact that RET_PF_RETRY happens to be '1' is mostly luck.  Returning a postive
> value other than '1' should work, but as called out by the comments for the enum,
> using '0' for CONTINUE isn't a hard requirement.  E.g. if for some reason we used
> '0' for RET_PF_RETRY, this code would break.

I think this function is an especially awful case, as it seems to be natural
to return a RET_PF_ value from a function named kvm_mmu_page_fault().

> 
>   * Note, all values must be greater than or equal to zero so as not to encroach
>   * on -errno return values.  Somewhat arbitrarily use '0' for CONTINUE, which
>   * will allow for efficient machine code when checking for CONTINUE, e.g.
>   * "TEST %rax, %rax, JNZ", as all "stop!" values are non-zero.
> 
> FWIW, you are far from the first person to complain about KVM's mostly-undocumented
> 0/1/-errno return encoding scheme.  The problems is that it's so pervasive
> throughout KVM, that in some cases it's not easy to understand if a function is
> actually using that scheme, or just happens to return similar values.  I.e.
> converting to enums (or #defines) would require a lot of work and churn.

I think it would be helpful to at least add comments to the functions
returning the 0/1/-errno value.

And it would be even better to have #defines for the 0 and 1. New use cases
should use the #defines, and whether we convert current users is another
question (I'd go for it, as it is only a minor additional work when adding
the comments anyway).

If you are fine with that, I can start the effort.


Juergen