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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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.

-- 
Vitaly