On Tue, Mar 21, 2023 at 08:53:55AM -0700, Sean Christopherson wrote:
> 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.
Argh, yeah. My route was just lazy.
> > 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);
>
LGTM, I'll get something like this incorporated for v2.
--
Thanks,
Oliver
