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>,
        "vkuznets@redhat.com" <vkuznets@redhat.com>,
        "mst@redhat.com" <mst@redhat.com>,
        Somdutta Roy <somduttar@nvidia.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH v2 3/3] KVM: x86: add vCPU ioctl for HLT exits disable
 capability
Thread-Topic: [RFC PATCH v2 3/3] KVM: x86: add vCPU ioctl for HLT exits
 disable capability
Thread-Index: AQHX9koEtqUfpyHZkkeby8KEcdrzF6xc3GqAgABfkBA=
Date:   Tue, 11 Jan 2022 06:35:49 +0000
Message-ID: <DM6PR12MB3500F288EB2952476E2F2E61CA519@DM6PR12MB3500.namprd12.prod.outlook.com>
References: <20211221090449.15337-1-kechenl@nvidia.com>
 <20211221090449.15337-4-kechenl@nvidia.com> <Ydyda6K8FrFveZX7@google.com>
In-Reply-To: <Ydyda6K8FrFveZX7@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?ZB/JqFJI10OdCYGsIzrmTyr6PmFaxpnrdUl5f/tTCJBmKKh4ZDoTYn01sY0K?=
 =?us-ascii?Q?aaPwzyelqdr0TAQQkTuyBzvOb0bHhzK/ogCbbdsPz3pKyFai8JjQP5+n+HWs?=
 =?us-ascii?Q?QgNOTDdfvWj6A67X6ga0deVWx0mM2wKLIL7moNxgXq20i34rIdGQcGN0Wiot?=
 =?us-ascii?Q?4/MiAOhgjHIJBBmaDe1XEtjy+PISF9l0vtmb7Tnd2DyKbtSQY73m9e5l9x27?=
 =?us-ascii?Q?PNHACOXCJyfzdyzQcWGTpADXynLuTGsKItJcmfEiSdYrIEvrKaJPaMiH7eIO?=
 =?us-ascii?Q?lzO8pd0FS6szvGoNyyE248boE9brB1tN4LCGyD6bZNI/3z4EtLQpRebK+JDF?=
 =?us-ascii?Q?/EYbkVBUKs14QsKMgR+mVNf+mWfPxC5qYOm0q2cDKxTVVzFsG0SWXZTdteh0?=
 =?us-ascii?Q?X1/Krv9ZCNhJw38rK3T2AbV0nknS9knoJmDkTosTBoW1rZypSdM8Ij/bnXOA?=
 =?us-ascii?Q?/rXecqt7u2Q2MOcAGkXjZQU9zk+n5bE7m1+s1/zuVZ7VzV9NS3HSTl8qVrjl?=
 =?us-ascii?Q?2bEgLQr48NLDBx98Zq8uqcGNnhbQyAigYaRE45+rQLZtzqQB/Kw7rQ6hrWzt?=
 =?us-ascii?Q?1svURjF7rfHulQhFcwt0ev4EX942wDwVkq9Brt26cDLJgy7z6Wkir3jl+Arq?=
 =?us-ascii?Q?saeCSlm80oKC9tDkGgSBoaSM1gIb5JekIlupeS/HRtULBHYTM8CWLKdkWVUO?=
 =?us-ascii?Q?BKDL4H/y+/bHjdGUkgVa4IpTm8NzXFID0u7i5uMyxGxIdHPRITE26ZgZEtTN?=
 =?us-ascii?Q?kPKFH1LoRsg7jXltFvvDB8drEIXt8XrupS4meyjr2fRkMd5RcO79W3K8UrBi?=
 =?us-ascii?Q?Jsv/xZp1qhJ7zlraRbq4fPbBQMf5Zf7E+dcbt5FJ0mH93F0N5qQ1rIzhMh78?=
 =?us-ascii?Q?6t+STKGdiZ0ZdL3cLUPpx7emQvxmSjnPkGCwrN39PYvPsXjFs4b6WwFxxdcS?=
 =?us-ascii?Q?kSnsvQA6b/0h79/9CSW7aJMVsnhhagTO6y9f5JUKebOh61nzuw34uxISw4MV?=
 =?us-ascii?Q?lOMbWlk7pkhcPK8iBZcsRYwiKfm0+63/s3SaNaqdaz2Ssu5x09pS5u4iNY6T?=
 =?us-ascii?Q?SUY4R0QMUDDNZF0FbTZux3ThhXCweZsWWYWVSmkf9fzs5nMKzdU0v+MU2NWQ?=
 =?us-ascii?Q?UGBNGim0saWSnoH+WKSFoClDTlg6KNaW0XdYO+CWU7u8bK7VNqWdev7AwP/W?=
 =?us-ascii?Q?nJTEbSD/ASn7GhtU7kQ5qiGZVy316G0WYju5AgZFNSIiAVOW+9dj71YPyXIR?=
 =?us-ascii?Q?iVPqqfMDae5VHEioGE3TCNxoOW7mIWUa3dGIs4a1g/xWblqb2U6u7dfzQehb?=
 =?us-ascii?Q?D4j0bexV4f1++yDj0lrUgMAPhWh692gQ66pePgxeRpod9LyZqejWk6RxZQTM?=
 =?us-ascii?Q?vJQVkC2XPSetva83SC5bLrdEdEhrcE1y0LCKP4Zpb39ozIPSx3Xb14hn2sll?=
 =?us-ascii?Q?sCdgeYK8z+50yHbh78jmIXwF6L1FFrS5ui0YC9h4YqFVR21qkWLc3kSn2znU?=
 =?us-ascii?Q?IvVZ1WLgstNvlCZVL8Cf0k3kmmi5IXXFs+2Wo1FcY3GQLjK2igMJ3ZBBUHi/?=
 =?us-ascii?Q?b9PA7av7Yvr65ggLGEQ+6UzCTCe1LPcpBIGxoPr7dgrDtfdsJATC1BWmzUOI?=
 =?us-ascii?Q?tUJaLpBgs3ILjXqQYqdCRGE=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: d870772a-5c25-463e-3900-08d9d4cc9b9c
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jan 2022 06:35:49.8134
 (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: QHAR/UrXfe318gFEmXI2nh7dkC+jaZdnESU+qJOxqg6AMl0l7rwCIzN3hjXUvQfjy63NixrBt+lD95bDAn0b3Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3033
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: Monday, January 10, 2022 12:56 PM
> To: Kechen Lu <kechenl@nvidia.com>
> Cc: kvm@vger.kernel.org; pbonzini@redhat.com; wanpengli@tencent.com;
> vkuznets@redhat.com; mst@redhat.com; Somdutta Roy
> <somduttar@nvidia.com>; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH v2 3/3] KVM: x86: add vCPU ioctl for HLT exits
> disable capability
>=20
> External email: Use caution opening links or attachments
>=20
>=20
> On Tue, Dec 21, 2021, Kechen Lu wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> > d5d0d99b584e..d7b4a3e360bb 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5072,6 +5072,18 @@ static int kvm_vcpu_ioctl_enable_cap(struct
> kvm_vcpu *vcpu,
> >                       kvm_update_pv_runtime(vcpu);
> >
> >               return 0;
> > +
> > +     case KVM_CAP_X86_DISABLE_EXITS:
> > +             if (cap->args[0] && (cap->args[0] &
> > +                             ~KVM_X86_DISABLE_VALID_EXITS))
>=20
> Bad alignment, but there's no need for the !0 in the first place, i.e.
>=20
>                 if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS)
>=20

Ack.

> but that's also incomplete as this path only supports toggling HLT, yet a=
llows
> all flavors of KVM_X86_DISABLE_VALID_EXITS.  Unless there's a good reason
> to not allow maniuplating the other exits, the proper fix is to just supp=
ort
> everything.
>=20

Makes sense. When I implement this patch version, only thinking about the u=
se case of
HLT intercept and want to see more comments if this framework looks good. W=
ill complete
this to support other exits.

> > +                     return -EINVAL;
> > +
> > +             vcpu->arch.hlt_in_guest =3D (cap->args[0] &
> > +                     KVM_X86_DISABLE_EXITS_HLT) ? true : false;
>=20
> Hmm, this behavior diverges from the per-VM ioctl, which doesn't allow re=
-
> enabling a disabled exit.  We can't change the per-VM behavior without
> breaking backwards compatibility, e.g. if userspace does:
>=20
>         if (allow_mwait)
>                 kvm_vm_disable_exit(KVM_X86_DISABLE_EXITS_MWAIT)
>         if (allow_hlt)
>                 kvm_vm_disable_exit(KVM_X86_DISABLE_EXITS_HLT)
>=20
> then changing KVM behavior would result in MWAIT behavior intercepted
> when previously it would have been allowed.  We have a userspace VMM
> that operates like this...
>=20
> Does your use case require toggling intercepts?  Or is the configuration
> static?
> If it's static, then the easiest thing would be to follow the per-VM beha=
vior so
> that there are no suprises.  If toggling is required, then I think the be=
st thing
> would be to add a prep patch to add an override flag to the per-VM ioctl,=
 and
> then share code between the per-VM and per-vCPU paths for modifying the
> flags (attached as patch 0003).
>=20

Our use case for now is static configuration. But since the per-vcpu ioctl =
is
anyhow executed runtime after the vcpu creation, so it is the "toggling" an=
d
needs overrides on some vcpus. "OVERRIDE" flag makes much sense to me,
the macro looks clean and neat for reducing redundant codes. Thanks a lot
for the patch.

> Somewhat related, there's another bug of sorts that I think we can safely=
 fix.
> KVM doesn't reject disabling of MWAIT exits when MWAIT isn't allowed in
> the guest, and instead ignores the bad input.  Not a big deal, but fixing=
 that
> means KVM doesn't need to check kvm_can_mwait_in_guest() when
> processing the args to update flags.  If that breaks someone's userspace,=
 the
> alternative would be to tweak the attached patch 0003 to introduce the
> OVERRIDE, e.g.
>=20
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> f611a49ceba4..3bac756bab79 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5053,6 +5053,8 @@ static int kvm_vcpu_ioctl_device_attr(struct
> kvm_vcpu *vcpu,
>=20
>  #define kvm_ioctl_disable_exits(a, mask)                                =
    \
>  ({                                                                      =
    \
> +       if (!kvm_can_mwait_in_guest())                                   =
    \
> +               (mask) &=3D KVM_X86_DISABLE_EXITS_MWAIT;                 =
      \

For some userspace's backward compatibility, adding this tweak to the attac=
hed
Patch 0003 makes sense. BTW, (mask) &=3D KVM_X86_DISABLE_EXITS_MWAIT
seems should be (mask) &=3D ~KVM_X86_DISABLE_EXITS_MWAIT, I guess it's a=20
typo :P. Will include the attached patch 0001 in the next as well. Thanks f=
or
all the help!

Best Regards,
Kechen

>         if ((mask) & KVM_X86_DISABLE_EXITS_OVERRIDE) {                   =
    \
>                 (a).mwait_in_guest =3D (mask) & KVM_X86_DISABLE_EXITS_MWA=
IT;
> \
>                 (a).hlt_in_guest =3D (mask) & KVM_X86_DISABLE_EXITS_HLT; =
      \
>=20
>=20
> If toggling is not required, then I still think it makes sense to add a m=
acro to
> handle propagating the capability args to the arch flags.