Re: [PATCH 01/11] KVM: x86: Redefine 'longmode' as a flag for KVM_EXIT_HYPERCALL

From: Sean Christopherson <seanjc@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Salil Mehta <salil.mehta@huawei.com>
Subject: Re: [PATCH 01/11] KVM: x86: Redefine 'longmode' as a flag for KVM_EXIT_HYPERCALL
Date: Tue, 21 Mar 2023 08:53:55 -0700	[thread overview]
Message-ID: <ZBnTE5hCGUOSK3BW@google.com> (raw)
In-Reply-To: <20230320221002.4191007-2-oliver.upton@linux.dev>

On Mon, Mar 20, 2023, Oliver Upton wrote:
> The 'longmode' field is a bit annoying as it blows an entire __u32 to
> represent a boolean value. Since other architectures are looking to add
> support for KVM_EXIT_HYPERCALL, now is probably a good time to clean it
> up.
> 
> Redefine the field (and the remaining padding) as a set of flags.
> Preserve the existing ABI by using the lower 32 bits of the field to
> indicate if the guest was in long mode at the time of the hypercall.

Setting all of bits 31:0 doesn't strictly preserve the ABI, e.g. will be a
breaking change if userspace does something truly silly like

	if (vcpu->run->hypercall.longmode == true)
		...

It's likely unnecessary paranoia, but at the same time it's easy to avoid.

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/x86/include/uapi/asm/kvm.h | 9 +++++++++
>  arch/x86/kvm/x86.c              | 5 ++++-
>  include/uapi/linux/kvm.h        | 9 +++++++--
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 7f467fe05d42..ab7b7b1d7c9d 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -559,4 +559,13 @@ struct kvm_pmu_event_filter {
>  #define KVM_VCPU_TSC_CTRL 0 /* control group for the timestamp counter (TSC) */
>  #define   KVM_VCPU_TSC_OFFSET 0 /* attribute for the TSC offset */
>  
> +/*
> + * x86-specific KVM_EXIT_HYPERCALL flags.
> + *
> + * KVM previously used a u32 field to indicate the hypercall was initiated from
> + * long mode. As such, the lower 32 bits of the flags are used for long mode to
> + * preserve ABI.
> + */
> +#define KVM_EXIT_HYPERCALL_LONG_MODE	GENMASK_ULL(31, 0)

For the uapi, I think it makes sense to do:

  #define KVM_EXIT_HYPERCALL_LONG_MODE	BIT(0)

and then somewhere internally do:

  /* Snarky comment goes here. */
  #define KVM_EXIT_HYPERCALL_MBZ	GENMASK_ULL(31, 1)

>  #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7713420abab0..c61c2b0c73bd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9803,7 +9803,10 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  		vcpu->run->hypercall.args[0]  = gpa;
>  		vcpu->run->hypercall.args[1]  = npages;
>  		vcpu->run->hypercall.args[2]  = attrs;
> -		vcpu->run->hypercall.longmode = op_64_bit;
> +		vcpu->run->hypercall.flags    = 0;
> +		if (op_64_bit)
> +			vcpu->run->hypercall.flags |= KVM_EXIT_HYPERCALL_LONG_MODE;
> +

And add a runtime assertion to make sure we don't botch this in the future:

		WARN_ON_ONCE(vcpu->run->hypercall.flags & KVM_EXIT_HYPERCALL_MBZ);

>  		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
>  		return 0;
>  	}