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

[Oliver Upton <oliver.upton@linux.dev>]

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@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);
> 

LGTM, I'll get something like this incorporated for v2.

-- 
Thanks,
Oliver