From:   Kechen Lu <kechenl@nvidia.com>
To:     Sean Christopherson <seanjc@google.com>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "wanpengli@tencent.com" <wanpengli@tencent.com>,
        "rkrcmar@redhat.com" <rkrcmar@redhat.com>,
        "vkuznets@redhat.com" <vkuznets@redhat.com>,
        Somdutta Roy <somduttar@nvidia.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH] KVM: x86: add kvm per-vCPU exits disable capability
Thread-Topic: [RFC PATCH] KVM: x86: add kvm per-vCPU exits disable capability
Thread-Index: AQHX8JtYjBglrMieUkS/P7QtoxwNoKwyV06AgAAdI3A=
Date:   Tue, 14 Dec 2021 21:07:36 +0000
Message-ID: <DM6PR12MB3500E1787271F74A4976143ECA759@DM6PR12MB3500.namprd12.prod.outlook.com>
References: <20211214033227.264714-1-kechenl@nvidia.com>
 <Ybjoy5h9a8nKK9X4@google.com>
In-Reply-To: <Ybjoy5h9a8nKK9X4@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?436rD4pOq9rQ2bAoIpr1ZHkDyKlkZpLAC5c39jAxO3WUBcPr4qOnIOFjuhm3?=
 =?us-ascii?Q?2Y4H3v5ObulV9cQPN3slPLj160EutwWJglkDWuzDxXqkG4Bsz0Rj7G5tTtkx?=
 =?us-ascii?Q?y4jhfdcj1OM6uJYpyaQ8+jsngTPCbWx1U0kposSMlZ2EHgx2Ja14lLj8btrk?=
 =?us-ascii?Q?q8sK8fIEEZ6QiA2U32sCEKm6zWF85orXXlbMNMTsLQ3AaC0kQMi6VVwUg1JL?=
 =?us-ascii?Q?CkrvYti0FoYoC+oeo6WL7Eu2R1rNht2ThGBPFuOW0Lay2JsjXhBpgkzmJHFX?=
 =?us-ascii?Q?SlV71OS4YkN/XNuj3Jcnc6lXuvqRRLtRQFbdDNb5+gbOTpZra4RpNeySmflN?=
 =?us-ascii?Q?lUBXkAmJIHBi632fDhoQs+BGgME3u3XGoty8lhiHRcXrzGGpnJDczaq9xfxN?=
 =?us-ascii?Q?TZZO0LZwIn9VGA8cGQmVaLXEvAexx3783CxDvSB523xx6/WHj5xC4COeqJE7?=
 =?us-ascii?Q?nV/NVjhjf7PE3pijeMZ1LA2i7kw6ZDy0GLEZ+SWHPsVofXDXpVaKXPd3KlZD?=
 =?us-ascii?Q?Or/MHZaIwRcLdABlgBsGjRd9SAbNdxnhccgbohT9sMGhUQb3HnTypHy3Otpk?=
 =?us-ascii?Q?5d76zKypSqpoNHjLIADPcTdFBq4H+MN5DKDa6oJttSOy6zhHd0/YDJMLhtPt?=
 =?us-ascii?Q?LYs4cnI9x4rGwE8EQMSfSyIUcXinXb87uDSP36p1ZPC4VCqTrMhIU9L59jKf?=
 =?us-ascii?Q?yNnBgtAeFib7KtxSGwT4sDAt3Fw+p3G3WPF/FmQ0vsRElVP2yaMahc59zD0A?=
 =?us-ascii?Q?slcSbt1/ayugeyLLR0hGsDd8ivdMIoX7dps6COtRaU7C4ZFxP1w0r4f+JLri?=
 =?us-ascii?Q?BZhaGSNqsJUW1QzZJnkt2QBJIt+/KzBLS/BCYmcwky9/1vV4AhKr+97ynTau?=
 =?us-ascii?Q?EiEwdKhAwlQVZAGWUQkeazJKDlAPe4OCsx+u6PKgEqj02OmTBIeh1ZB4j/Xt?=
 =?us-ascii?Q?Y7QGNGQDXYdSXGCk0xDDR3Yia86+S6QVOSshYcALtHoi1NTS4G2Xx4+X0AKr?=
 =?us-ascii?Q?DvCDrF0C6pxnsbO9PPhEc9UFK9RtLO+IhU7xvik9KU/XY8GudZxFDc3swb+F?=
 =?us-ascii?Q?j369Tlb0Ma/CxtxCHR0Fjznj1+XbtYv5eM9JUyT/nid5ficQuflmDhZ2eDRK?=
 =?us-ascii?Q?2zLNUjEbQ0Gl1tJTWCX1MEKN7oh0LquDowZIJKHFUfRO04SiOtuOdP7DPB2Y?=
 =?us-ascii?Q?389DIfbArui1o1vhp/9CeY/qKC0hnYJPRGrtlauEqzT+GPTsEGoWfFSMPJlN?=
 =?us-ascii?Q?fxxBNGeBdUoelCTmP4onv62s6Zolo4aUCcNvMTlBthoutoxKYBhilRX5Vzic?=
 =?us-ascii?Q?inG9Zc1du8gBmrYZ9sgUxdKx6oeWjtefZuTKN7AuylNLYOxAk2Sb5ISg7UGZ?=
 =?us-ascii?Q?dUok3nOBdmeyeYzTnSQtSpXW8ZRd1EHqZDsYQJtchK8O2ZfISdvX0yoM1l5g?=
 =?us-ascii?Q?s/bT2ewdzQq+HCHxJzdF4n5MAx9/NHGRzwh5gadU43/+XOqUcmHSf1astVb4?=
 =?us-ascii?Q?Gsk3uHFI8GPPiZRZ3zIAvBpeiBU4wbhXhYPp1owfRg3dJAntZ0+lsQ5CGWjY?=
 =?us-ascii?Q?nBKTgDp2jgr4dtKOVSMyoMxdBBjpneTbd8EZBoHPjihgh6wJUzSxOdPSSiqD?=
 =?us-ascii?Q?JGSnUbY640Skz41V9jvH5fg=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3500.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49583caf-6a06-408b-a0c3-08d9bf45c128
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Dec 2021 21:07:36.2856
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: wP1o0WAnrLaAYb5F1MefPlLplBbt3BwKiVoevzwOcw4N52uNiyVuwryfb7RO36mEZwzBdv8dS2131Ggc+jcm1A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1513
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Tuesday, December 14, 2021 10:56 AM
> To: Kechen Lu <kechenl@nvidia.com>
> Cc: kvm@vger.kernel.org; pbonzini@redhat.com; wanpengli@tencent.com;
> rkrcmar@redhat.com; vkuznets@redhat.com; Somdutta Roy
> <somduttar@nvidia.com>; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH] KVM: x86: add kvm per-vCPU exits disable
> capability
>=20
> External email: Use caution opening links or attachments
>=20
>=20
> 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)
>=20
> 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 support=
ing
> a vCPU-scoped ioctl().
>=20

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

From my understanding, so we could add a new vcpu ioctrl e.g. KVM_DISABLE_E=
XITS, 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.=20

> 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.
>=20
> Probably worth formalizing that requirement too, e.g.
>=20
> 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;
>=20
> +               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 =3D true; @@ -5785,6 +57=
89,8 @@ int
> kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>                 if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE)
>                         kvm->arch.cstate_in_guest =3D true;
>                 r =3D 0;
> +disable_exits_unlock:
> +               mutex_unlock(&kvm->lock);
>                 break;
>         case KVM_CAP_MSR_PLATFORM_INFO:
>                 kvm->arch.guest_can_read_msr_platform_info =3D cap->args[=
0];

Got it. Will add this explicit requirement check as a subpatch in next vers=
ion.

Thanks for the quick review!

Best Regards,
Kechen