RE: [RFC PATCH v2 1/3] KVM: x86: only allow exits disable before vCPUs created

[Kechen Lu]

Hi Sean,

> -----Original Message-----
> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Sent: Monday, January 10, 2022 10:50 AM
> To: Kechen Lu <kechenl@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; wanpengli@xxxxxxxxxxx;
> vkuznets@xxxxxxxxxx; mst@xxxxxxxxxx; Somdutta Roy
> <somduttar@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH v2 1/3] KVM: x86: only allow exits disable before
> vCPUs created
> 
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Dec 21, 2021, Kechen Lu wrote:
> > Since VMX and SVM both would never update the control bits if exits
> > are disable after vCPUs are created, only allow setting exits disable
> > flag before vCPU creation.
> >
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> 
> For this to carry my SOB, I should be attributed as the author, or add a
> Co-developed-by: for me.  I'm also totally ok with a Suggested-by: or
> Reported-by:
> 

My apologies for putting incorrect SOB format :P Will fix it!

> And we should at least have
> 
>   Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT
> intercepts")
> 

Ack! Will mention it in the description.

> andy maybe Cc: stable@xxxxxxxxxxxxxxx, though I'm not entirely sure this is
> stable material as it could in theory do more harm than good if there's a
> busted userspace out there.
> 

I see, will cc stable mailing list. IMO with this patch, incorrect behavior from userspace
only cause the set flag "ineffective", not sure if this breaks some userspace seriously.

Best Regards,
Kechen

> If this doesn't carry my SOB...
> 
> Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> 
> > Signed-off-by: Kechen Lu <kechenl@xxxxxxxxxx>
> > ---
> >  Documentation/virt/kvm/api.rst | 1 +
> >  arch/x86/kvm/x86.c             | 6 ++++++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst index aeeb071c7688..d1c50b95bbc1
> > 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -6581,6 +6581,7 @@ branch to guests' 0x200 interrupt vector.
> >  :Architectures: x86
> >  :Parameters: args[0] defines which exits are disabled
> >  :Returns: 0 on success, -EINVAL when args[0] contains invalid exits
> > +          or if any vCPU has already been created
> >
> >  Valid bits in args[0] are::
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> > 0cf1082455df..37529c0c279d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5764,6 +5764,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; @@ -5774,6
> > +5778,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];
> > --
> > 2.30.2
> >