Re: [PATCH 05/10] KVM/x86: Add KVM_MSR_RET_* defines for values 0 and 1

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

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

On 05.12.25 11:23, Vitaly Kuznetsov wrote:
> Juergen Gross <jgross@suse.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@suse.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

[-- 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 --]