From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC405C761A6 for ; Thu, 6 Apr 2023 03:29:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233867AbjDFD3b (ORCPT ); Wed, 5 Apr 2023 23:29:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43914 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230059AbjDFD32 (ORCPT ); Wed, 5 Apr 2023 23:29:28 -0400 Received: from mail-pl1-x649.google.com (mail-pl1-x649.google.com [IPv6:2607:f8b0:4864:20::649]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4A7C83C3E for ; Wed, 5 Apr 2023 20:29:27 -0700 (PDT) Received: by mail-pl1-x649.google.com with SMTP id l11-20020a170902d34b00b001a4ee9558fcso749519plk.8 for ; Wed, 05 Apr 2023 20:29:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680751767; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=h15CORNu9ncNp+50JVcQuXVD3PLOS7VZQZXoeAXoypI=; b=YKrkA/XjuX6OGTnY3lclc41AWxFCIYXsmF5hsRRCLlmr/LZ/haCyTj+ldKjCXP8dx/ Xq2z7o5zczfJZ9cf8f60rjTXF041Ij3mBapglLv595BKKT1dHfwz8ajiynXQgNNrpPED RnwEe4tF62B4cDtts0qC/qqvSbw0qTJHgGROg3d+Z4cSQkFptlm3vuTRtZMgf+TmnpP/ 9tGxwCI5eHvVZZW+dtgQV1vu5M3DSvVWbeE6uS7RLNUN1mCSf5RSfdJ4xRmI2+n1pGmJ /6D7+hEO8OeHOXEzIgx8VLIOe8wkCFSqtvoc7+/n8OwTC9wqC1LRubCSVkKe7NXzzfwG mZ6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680751767; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=h15CORNu9ncNp+50JVcQuXVD3PLOS7VZQZXoeAXoypI=; b=S8Ck+e6n2zgUe1diRlUwAb0HoiNEkFneBwL03JSPBj4TcpmNOnOzBj/7LrCnkpWCwT 9tFVnWKFtKcDMYP9RvjCe4/vxNHvg7go0W904g9ee6EKgUAATc1jz0rsFAmKUh7+fxBB U5dW1Y6HdQkDOlCm5nsA08WdDMGnjq/nDpdTyRzPkvjIheEiOv/PFdAgzDXCZbd10b3N FeXqMQD3jl73g0dpE5qU87XD+XAoMst7h3AzXS28vTYhCcSF4D+BrmIhXQjztKlU92Ll 3LgShKeIiYvkyW3nCmIyOV3cR6OrVGA9xCcH+jfyhSgPnm+qv2kkrJBvK0NMid5KPk37 vL9Q== X-Gm-Message-State: AAQBX9eGuIzwI+mkMGSCm5kUnry6inm3yBBh+7Pk0QfbuoySCWISlJd5 YbQo2aDkF8fbRdJnG/s+eSY6tRsjLjQ= X-Google-Smtp-Source: AKy350Y5pJ/sdE/HZdSCmlnnToA2XpgFqtC6nkm7umunJfj4QTc3Sw1KtIh0wSIpP63Yb2wWDL9RYav5Qdc= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:9a8b:b0:1a0:6000:7fd0 with SMTP id w11-20020a1709029a8b00b001a060007fd0mr3552643plp.5.1680751766855; Wed, 05 Apr 2023 20:29:26 -0700 (PDT) Date: Wed, 5 Apr 2023 20:29:25 -0700 In-Reply-To: <9fbf5b4022d67157d6305bc1811f36d9096c26fc.1680179693.git.houwenlong.hwl@antgroup.com> Mime-Version: 1.0 References: <9227068821b275ac547eb2ede09ec65d2281fe07.1680179693.git.houwenlong.hwl@antgroup.com> <9fbf5b4022d67157d6305bc1811f36d9096c26fc.1680179693.git.houwenlong.hwl@antgroup.com> Message-ID: Subject: Re: [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running From: Sean Christopherson To: Hou Wenlong Cc: kvm@vger.kernel.org, Paolo Bonzini , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , linux-kernel@vger.kernel.org, Kechen Lu Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +Kechen On Thu, Mar 30, 2023, Hou Wenlong wrote: > __kvm_update_cpuid_runtime() may be called during vCPU running and KVM > PV feature CPUID is updated too. But the cached KVM PV feature bitmap is > not updated. Actually, KVM PV feature CPUID shouldn't be updated, > otherwise, KVM PV feature would be broken in guest. Currently, only > KVM_FEATURE_PV_UNHALT is updated, and it's impossible after disallow > disable HLT exits. However, KVM PV feature CPUID should be updated only > in KVM_SET_CPUID{,2} ioctl. > > Signed-off-by: Hou Wenlong > --- > arch/x86/kvm/cpuid.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 6972e0be60fa..af92d3422c79 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -222,6 +222,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp > vcpu->arch.cpuid_nent); > } > > +static void kvm_update_pv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, > + int nent) > +{ > + struct kvm_cpuid_entry2 *best; > + > + best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); > + if (kvm_hlt_in_guest(vcpu->kvm) && best && > + (best->eax & (1 << KVM_FEATURE_PV_UNHALT))) > + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > +} > + > void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) > { > struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu); > @@ -280,11 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > cpuid_entry_has(best, X86_FEATURE_XSAVEC))) > best->ebx = xstate_required_size(vcpu->arch.xcr0, true); > > - best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); > - if (kvm_hlt_in_guest(vcpu->kvm) && best && > - (best->eax & (1 << KVM_FEATURE_PV_UNHALT))) > - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > - > if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { > best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); > if (best) > @@ -402,6 +408,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > int r; > > __kvm_update_cpuid_runtime(vcpu, e2, nent); > + kvm_update_pv_cpuid(vcpu, e2, nent); Hrm, this will silently conflict with the proposed per-vCPU controls[*]. Though arguably that patch is buggy and "needs" to toggle PV_UNHALT when userspace messes with HLT passthrough. But that doesn't really make sense either because no guest will react kindly to KVM_FEATURE_PV_UNHALT disappearing. I really wish this code didn't exist, i.e. that KVM let/forced userspace deal with correctly defining guest CPUID. Kechen, is it feasible for your userspace to clear PV_UNHALT when it (might) use the per-vCPU control? I.e. can KVM do as this series proposes and update guest CPUID only on KVM_SET_CPUID{2}? Dropping the behavior for the per-VM control is probably not an option as I gotta assume that'd break userspace, but I would really like to avoid carrying that over to the per-vCPU control, which would get quite messy and probably can't work anyways. [*] https://lkml.kernel.org/r/20230121020738.2973-6-kechenl%40nvidia.com