[v2] KVM/x86: Drop "1" as MSR emulation return value

[PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Juergen Gross 1 week, 4 days ago

Get rid of the literal "1" used as general error return value in KVM
MSR emulation. It can easily be replaced by negative errno values
instead.

This is meant to avoid confusion with the literal "1" used as return
value for "return to guest".

Changes in V2:
- series carved out from initial "KVM: Avoid literal numbers as return
  values" series
- don't use new KVM_MSR_RET_* defines, but 0 and -errno

Juergen Gross (6):
  KVM/x86: Change comment before KVM_MSR_RET_* defines
  KVM/x86: Return -errno instead of "1" for APIC related MSR emulation
  KVM/x86: Return -errno instead of "1" for Hyper-V related MSR
    emulation
  KVM/x86: Return -errno instead of "1" for VMX related MSR emulation
  KVM/x86: Return -errno instead of "1" for SVM related MSR emulation
  KVM/x86: Return -errno instead of "1" for common MSR emulation

 arch/x86/kvm/hyperv.c        |  72 ++++++++++++-------------
 arch/x86/kvm/lapic.c         |  39 +++++++-------
 arch/x86/kvm/mtrr.c          |   6 +--
 arch/x86/kvm/pmu.c           |   8 +--
 arch/x86/kvm/svm/pmu.c       |   4 +-
 arch/x86/kvm/svm/svm.c       |  36 ++++++-------
 arch/x86/kvm/vmx/nested.c    |   2 +-
 arch/x86/kvm/vmx/pmu_intel.c |  16 +++---
 arch/x86/kvm/vmx/tdx.c       |  10 ++--
 arch/x86/kvm/vmx/vmx.c       |  96 ++++++++++++++++-----------------
 arch/x86/kvm/x86.c           | 102 +++++++++++++++++------------------
 arch/x86/kvm/x86.h           |   4 +-
 arch/x86/kvm/xen.c           |  10 ++--
 13 files changed, 202 insertions(+), 203 deletions(-)

-- 
2.54.0

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Juergen Gross 1 week, 4 days ago

Please disregard this series, there is one complication sashiko made me
aware of.


Juergen

On 28.05.26 13:35, Juergen Gross wrote:
> Get rid of the literal "1" used as general error return value in KVM
> MSR emulation. It can easily be replaced by negative errno values
> instead.
> 
> This is meant to avoid confusion with the literal "1" used as return
> value for "return to guest".
> 
> Changes in V2:
> - series carved out from initial "KVM: Avoid literal numbers as return
>    values" series
> - don't use new KVM_MSR_RET_* defines, but 0 and -errno
> 
> Juergen Gross (6):
>    KVM/x86: Change comment before KVM_MSR_RET_* defines
>    KVM/x86: Return -errno instead of "1" for APIC related MSR emulation
>    KVM/x86: Return -errno instead of "1" for Hyper-V related MSR
>      emulation
>    KVM/x86: Return -errno instead of "1" for VMX related MSR emulation
>    KVM/x86: Return -errno instead of "1" for SVM related MSR emulation
>    KVM/x86: Return -errno instead of "1" for common MSR emulation
> 
>   arch/x86/kvm/hyperv.c        |  72 ++++++++++++-------------
>   arch/x86/kvm/lapic.c         |  39 +++++++-------
>   arch/x86/kvm/mtrr.c          |   6 +--
>   arch/x86/kvm/pmu.c           |   8 +--
>   arch/x86/kvm/svm/pmu.c       |   4 +-
>   arch/x86/kvm/svm/svm.c       |  36 ++++++-------
>   arch/x86/kvm/vmx/nested.c    |   2 +-
>   arch/x86/kvm/vmx/pmu_intel.c |  16 +++---
>   arch/x86/kvm/vmx/tdx.c       |  10 ++--
>   arch/x86/kvm/vmx/vmx.c       |  96 ++++++++++++++++-----------------
>   arch/x86/kvm/x86.c           | 102 +++++++++++++++++------------------
>   arch/x86/kvm/x86.h           |   4 +-
>   arch/x86/kvm/xen.c           |  10 ++--
>   13 files changed, 202 insertions(+), 203 deletions(-)
>

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Sean Christopherson 1 week, 4 days ago

On Thu, May 28, 2026, Juergen Gross wrote:
> Please disregard this series, there is one complication sashiko made me
> aware of.

Sashiko beat me to the punch. :-)

See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
for a real world example of how things can and will go wrong.

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Jürgen Groß 1 week, 4 days ago

On 28.05.26 15:09, Sean Christopherson wrote:
> On Thu, May 28, 2026, Juergen Gross wrote:
>> Please disregard this series, there is one complication sashiko made me
>> aware of.
> 
> Sashiko beat me to the punch. :-)
> 
> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> for a real world example of how things can and will go wrong.

Yeah, with Sashiko's pointer it was easy to spot.

Question now is whether the already existing cases of -errno passed as return
value are wrong or on purpose. If the latter, there should be a comment for
that, otherwise they need to be fixed..

Disentangling the MSR emulation return values from the "normal" ones ("return
to guest"/"return to user mode") will be quite interesting with the overloaded
semantics of "1".

Juergen

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Sean Christopherson 1 week, 4 days ago

On Thu, May 28, 2026, Jürgen Groß wrote:
> On 28.05.26 15:09, Sean Christopherson wrote:
> > On Thu, May 28, 2026, Juergen Gross wrote:
> > > Please disregard this series, there is one complication sashiko made me
> > > aware of.
> > 
> > Sashiko beat me to the punch. :-)
> > 
> > See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> > for a real world example of how things can and will go wrong.
> 
> Yeah, with Sashiko's pointer it was easy to spot.
> 
> Question now is whether the already existing cases of -errno passed as return
> value are wrong or on purpose. 

What are the existing cases?

> If the latter, there should be a comment for
> that, otherwise they need to be fixed..
> 
> Disentangling the MSR emulation return values from the "normal" ones ("return
> to guest"/"return to user mode") will be quite interesting with the overloaded
> semantics of "1".

LOL, "interesting".

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Jürgen Groß 1 week, 3 days ago

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?
> 
>> If the latter, there should be a comment for
>> that, otherwise they need to be fixed..
>>
>> Disentangling the MSR emulation return values from the "normal" ones ("return
>> to guest"/"return to user mode") will be quite interesting with the overloaded
>> semantics of "1".
> 
> LOL, "interesting".

What do you think about the following idea:

Lets pass struct msr_info * down to all functions which get their return
value passed up. Then extend msr_info with a bool "return_to_guest" (valid
only if !host_initiated), which should be set instead of passing "1" up to
the caller (probably using an inline helper). Then the return value could
be 0 or -errno, and after MSR emulation the return_to_guest indicator can
be tested if needed.


Juergen

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Juergen Gross 1 week, 3 days ago

On 28.05.26 17:50, Jürgen Groß wrote:
> On 28.05.26 15:21, Sean Christopherson wrote:
>> On Thu, May 28, 2026, Jürgen Groß wrote:
>>> On 28.05.26 15:09, Sean Christopherson wrote:
>>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>>> Please disregard this series, there is one complication sashiko made me
>>>>> aware of.
>>>>
>>>> Sashiko beat me to the punch. :-)
>>>>
>>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad 
>>>> WRMSR(MCi_CTL/STATUS)")
>>>> for a real world example of how things can and will go wrong.
>>>
>>> Yeah, with Sashiko's pointer it was easy to spot.
>>>
>>> Question now is whether the already existing cases of -errno passed as return
>>> value are wrong or on purpose.
>>
>> What are the existing cases?
>>
>>> If the latter, there should be a comment for
>>> that, otherwise they need to be fixed..
>>>
>>> Disentangling the MSR emulation return values from the "normal" ones ("return
>>> to guest"/"return to user mode") will be quite interesting with the overloaded
>>> semantics of "1".
>>
>> LOL, "interesting".
> 
> What do you think about the following idea:
> 
> Lets pass struct msr_info * down to all functions which get their return
> value passed up. Then extend msr_info with a bool "return_to_guest" (valid
> only if !host_initiated), which should be set instead of passing "1" up to
> the caller (probably using an inline helper). Then the return value could
> be 0 or -errno, and after MSR emulation the return_to_guest indicator can
> be tested if needed.

In the end I think it is less error prone to define a struct kvm_msr_ret_t
used as return type, consisting of an int and a bool with the same semantics
as above. Helpers will still be a good idea IMHO.

I'll have a try how it looks like.


Juergen

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Jürgen Groß 1 week, 4 days ago

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?

Found another one:

kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())


Juergen

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by David Woodhouse 1 week, 3 days ago

On Thu, 2026-05-28 at 16:33 +0200, Jürgen Groß wrote:
> On 28.05.26 15:21, Sean Christopherson wrote:
> > On Thu, May 28, 2026, Jürgen Groß wrote:
> > > On 28.05.26 15:09, Sean Christopherson wrote:
> > > > On Thu, May 28, 2026, Juergen Gross wrote:
> > > > > Please disregard this series, there is one complication sashiko made me
> > > > > aware of.
> > > > 
> > > > Sashiko beat me to the punch. :-)
> > > > 
> > > > See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> > > > for a real world example of how things can and will go wrong.
> > > 
> > > Yeah, with Sashiko's pointer it was easy to spot.
> > > 
> > > Question now is whether the already existing cases of -errno passed as return
> > > value are wrong or on purpose.
> > 
> > What are the existing cases?
> 
> Found another one:
> 
> kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())

You mean in the case where it's using the user-provided hypercall page,
and can't copy from the buffer that the VMM provided?

I think that's correct to return -errno via PTR_ERR() and let the guest
die?

The rest return 0 or 1.

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Jürgen Groß 1 week, 3 days ago

On 28.05.26 17:32, David Woodhouse wrote:
> On Thu, 2026-05-28 at 16:33 +0200, Jürgen Groß wrote:
>> On 28.05.26 15:21, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Jürgen Groß wrote:
>>>> On 28.05.26 15:09, Sean Christopherson wrote:
>>>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>>>> Please disregard this series, there is one complication sashiko made me
>>>>>> aware of.
>>>>>
>>>>> Sashiko beat me to the punch. :-)
>>>>>
>>>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>>>> for a real world example of how things can and will go wrong.
>>>>
>>>> Yeah, with Sashiko's pointer it was easy to spot.
>>>>
>>>> Question now is whether the already existing cases of -errno passed as return
>>>> value are wrong or on purpose.
>>>
>>> What are the existing cases?
>>
>> Found another one:
>>
>> kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())
> 
> You mean in the case where it's using the user-provided hypercall page,
> and can't copy from the buffer that the VMM provided?

Yes.

> 
> I think that's correct to return -errno via PTR_ERR() and let the guest
> die?

In this case I think a comment in this regard would be nice, as it would
prevent others stumbling over it asking the same question again.


Juergen

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

Posted by Jürgen Groß 1 week, 4 days ago

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?

Have a look at:

kvm_hv_msr_get_crash_data()
kvm_hv_msr_set_crash_data()
svm_get_msr()
svm_set_msr()


Juergen