linux-kernel - Re: [PATCH 05/10] KVM/x86: Add KVM_MSR_RET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <34048508-8ddd-4183-9d0d-f495af1984ab@suse.com>
Date: Fri, 5 Dec 2025 11:39:38 +0100
From: Jürgen Groß <jgross@...e.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>, linux-kernel@...r.kernel.org,
 x86@...nel.org, kvm@...r.kernel.org
Cc: Sean Christopherson <seanjc@...gle.com>,
 Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 05/10] KVM/x86: Add KVM_MSR_RET_* defines for values 0 and
 1

On 05.12.25 11:23, Vitaly Kuznetsov wrote:
> Juergen Gross <jgross@...e.com> writes:
> 
>> For MSR emulation return values only 2 special cases have defines,
>> while the most used values 0 and 1 don't.
>>
>> Reason seems to be the maze of function calls of MSR emulation
>> intertwined with the KVM guest exit handlers, which are using the
>> values 0 and 1 for other purposes. This even led to the comment above
>> the already existing defines, warning to use the values 0 and 1 (and
>> negative errno values) in the MSR emulation at all.
>>
>> Fact is that MSR emulation and exit handlers are in fact rather well
>> distinct, with only very few exceptions which are handled in a sane
>> way.
>>
>> So add defines for 0 and 1 values of MSR emulation and at the same
>> time comments where exit handlers are calling into MSR emulation.
>>
>> The new defines will be used later.
>>
>> No change of functionality intended.
>>
>> Signed-off-by: Juergen Gross <jgross@...e.com>
>> ---
>>   arch/x86/kvm/x86.c |  2 ++
>>   arch/x86/kvm/x86.h | 10 ++++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e733cb923312..e87963a47aa5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2130,6 +2130,7 @@ static int __kvm_emulate_rdmsr(struct kvm_vcpu *vcpu, u32 msr, int reg,
>>   	u64 data;
>>   	int r;
>>   
>> +	/* Call MSR emulation. */
>>   	r = kvm_emulate_msr_read(vcpu, msr, &data);
>>   
>>   	if (!r) {
>> @@ -2171,6 +2172,7 @@ static int __kvm_emulate_wrmsr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>   {
>>   	int r;
>>   
>> +	/* Call MSR emulation. */
>>   	r = kvm_emulate_msr_write(vcpu, msr, data);
>>   	if (!r) {
>>   		trace_kvm_msr_write(msr, data);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index f3dc77f006f9..e44b6373b106 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -639,15 +639,21 @@ enum kvm_msr_access {
>>   /*
>>    * Internal error codes that are used to indicate that MSR emulation encountered
>>    * an error that should result in #GP in the guest, unless userspace handles it.
>> - * Note, '1', '0', and negative numbers are off limits, as they are used by KVM
>> - * as part of KVM's lightly documented internal KVM_RUN return codes.
>> + * Note, negative errno values are possible for return values, too.
>> + * In case MSR emulation is called from an exit handler, any return value other
>> + * than KVM_MSR_RET_OK will normally result in a GP in the guest.
>>    *
>> + * OK		- Emulation succeeded. Must be 0, as in some cases return values
>> + *		  of functions returning 0 or -errno will just be passed on.
>> + * ERR		- Some error occurred.
>>    * UNSUPPORTED	- The MSR isn't supported, either because it is completely
>>    *		  unknown to KVM, or because the MSR should not exist according
>>    *		  to the vCPU model.
>>    *
>>    * FILTERED	- Access to the MSR is denied by a userspace MSR filter.
>>    */
>> +#define  KVM_MSR_RET_OK			0
>> +#define  KVM_MSR_RET_ERR		1
>>   #define  KVM_MSR_RET_UNSUPPORTED	2
>>   #define  KVM_MSR_RET_FILTERED		3
> 
> I like the general idea of the series as 1/0 can indeed be
> confusing. What I'm wondering is if we can do better by changing 'int'
> return type to something else. I.e. if the result of the function can be
> 'passed on' and KVM_MSR_RET_OK/KVM_MSR_RET_ERR have one meaning while
> KVM_MSR_RET_UNSUPPORTED/KVM_MSR_RET_FILTERED have another, maybe we can
> do better by changing the return type to something and then, when the
> value needs to be passed on, do proper explicit vetting of the result
> (e.g. to make sure only 1/0 pass through)? Just a thought, I think the
> series as-is makes things better and we can go with it for now.

The pass through case is always 0 or -errno, never the "1" (and of course
KVM_MSR_RET_*/-errno).

Changing from int to something else would probably require some helpers
for e.g. stuffing something like -EINVAL into it. An enum alone wouldn't
work for this, so it would need to be a specific new type, like a union
of an int (for the -errno) and an enum, but I believe this would make the
code harder to read instead of improving it.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)