Re: [PATCH v4 2/5] KVM: x86: Add a VALID_MASK for the MSR exit reason flags

[Sean Christopherson <seanjc@google.com>]

On Wed, Sep 21, 2022, Aaron Lewis wrote:
> Add the mask KVM_MSR_EXIT_REASON_VALID_MASK for the MSR exit reason

Uber nit, "the mask" is rather redundant, e.g.

Add KVM_MSR_EXIT_REASON_VALID_MASK to track the allowed MSR exit reason
flags.

> flags.  This simplifies checks that validate these flags, and makes it
> easier to introduce new flags in the future.
> 
> No functional change intended.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/x86.c       | 4 +---
>  include/uapi/linux/kvm.h | 3 +++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d7374d768296..852614246825 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6182,9 +6182,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		break;
>  	case KVM_CAP_X86_USER_SPACE_MSR:
>  		r = -EINVAL;
> -		if (cap->args[0] & ~(KVM_MSR_EXIT_REASON_INVAL |
> -				     KVM_MSR_EXIT_REASON_UNKNOWN |
> -				     KVM_MSR_EXIT_REASON_FILTER))
> +		if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK)
>  			break;
>  		kvm->arch.user_space_msr_mask = cap->args[0];
>  		r = 0;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index eed0315a77a6..44d476c3143a 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -485,6 +485,9 @@ struct kvm_run {
>  #define KVM_MSR_EXIT_REASON_INVAL	(1 << 0)
>  #define KVM_MSR_EXIT_REASON_UNKNOWN	(1 << 1)
>  #define KVM_MSR_EXIT_REASON_FILTER	(1 << 2)
> +#define KVM_MSR_EXIT_REASON_VALID_MASK	(KVM_MSR_EXIT_REASON_INVAL   |	\
> +					 KVM_MSR_EXIT_REASON_UNKNOWN |	\
> +					 KVM_MSR_EXIT_REASON_FILTER)

Put all of these VALID_MASK defines in arch/x86/include/asm/kvm_host.h so that
they aren't exposed to userspace, e.g. see KVM_X86_NOTIFY_VMEXIT_VALID_BITS.
Generally speaking, things should be kept in-kernel unless there's an actual need
to expose something to userspace.  Once something is exposed to userspace, our
options become much more limited.

E.g. if userspace does something silly like:

	filters = KVM_MSR_EXIT_REASON_VALID_MASK;

then upgrading kernel headers will unexpectedly change and potentially break
userspace, and then KVM is stuck having a bogus VALID_MASK.