RE: [RFC PATCH] KVM: x86: add kvm per-vCPU exits disable capability

[Kechen Lu]

> -----Original Message-----
> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Sent: Tuesday, December 14, 2021 10:56 AM
> To: Kechen Lu <kechenl@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; wanpengli@xxxxxxxxxxx;
> rkrcmar@xxxxxxxxxx; vkuznets@xxxxxxxxxx; Somdutta Roy
> <somduttar@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH] KVM: x86: add kvm per-vCPU exits disable
> capability
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Dec 13, 2021, Kechen Lu wrote:
> > ---
> >  Documentation/virt/kvm/api.rst  | 8 +++++++-
> > arch/x86/include/asm/kvm_host.h | 1 +
> >  arch/x86/kvm/cpuid.c            | 2 +-
> >  arch/x86/kvm/svm/svm.c          | 2 +-
> >  arch/x86/kvm/vmx/vmx.c          | 4 ++--
> >  arch/x86/kvm/x86.c              | 5 ++++-
> >  arch/x86/kvm/x86.h              | 5 +++--
> >  include/uapi/linux/kvm.h        | 4 +++-
> >  8 files changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst index aeeb071c7688..9a44896dc950
> > 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -6580,6 +6580,9 @@ branch to guests' 0x200 interrupt vector.
> >
> >  :Architectures: x86
> >  :Parameters: args[0] defines which exits are disabled
> > +             args[1] defines vCPU bitmask based on vCPU ID, 1 on
> > +                     corresponding vCPU ID bit would enable exists
> > +                     on that vCPU
> >  :Returns: 0 on success, -EINVAL when args[0] contains invalid exits
> >
> >  Valid bits in args[0] are::
> > @@ -6588,13 +6591,16 @@ Valid bits in args[0] are::
> >    #define KVM_X86_DISABLE_EXITS_HLT              (1 << 1)
> >    #define KVM_X86_DISABLE_EXITS_PAUSE            (1 << 2)
> >    #define KVM_X86_DISABLE_EXITS_CSTATE           (1 << 3)
> > +  #define KVM_X86_DISABLE_EXITS_PER_VCPU         (1UL << 63)
> 
> This doesn't scale, there are already plenty of use cases for VMs with 65+
> vCPUs.
> At a glance, I don't see anything fundamentally wrong with simply supporting
> a vCPU-scoped ioctl().
> 

Yeah, scale is the problem for using a 64bit mask, so in this RFC patch I use rol64(1UL, vcpu->vcpu_id) to rotating left shift the vCPU ID bits. But seems doing it through vcpu ioctl makes more sense.

>From my understanding, so we could add a new vcpu ioctrl e.g. KVM_DISABLE_EXITS, with parameter flag which exists to be disabled, e.g. struct exits_in_guest (in), or even without parameter just like you mentioned, pick up the per-VM cap flag the userspace set. 

> The VM-scoped version already has an undocumented requirement that it
> be called before vCPUs are created, because neither VMX nor SVM will
> update the controls if exits are disabled after vCPUs are created.  That means
> the flags checked at runtime can be purely vCPU, with the per-VM flag
> picked up at vCPU creation.
> 
> Probably worth formalizing that requirement too, e.g.
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> 85127b3e3690..6c9bc022a522 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5775,6 +5775,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>                 if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS)
>                         break;
> 
> +               mutex_lock(&kvm->lock);
> +               if (kvm->created_vcpus)
> +                       goto disable_exits_unlock;
> +
>                 if ((cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) &&
>                         kvm_can_mwait_in_guest())
>                         kvm->arch.mwait_in_guest = true; @@ -5785,6 +5789,8 @@ int
> kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>                 if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE)
>                         kvm->arch.cstate_in_guest = true;
>                 r = 0;
> +disable_exits_unlock:
> +               mutex_unlock(&kvm->lock);
>                 break;
>         case KVM_CAP_MSR_PLATFORM_INFO:
>                 kvm->arch.guest_can_read_msr_platform_info = cap->args[0];

Got it. Will add this explicit requirement check as a subpatch in next version.

Thanks for the quick review!

Best Regards,
Kechen