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@xxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> ---
> 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;
> }
