Message ID | 20230503182852.3431281-2-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1535100vqo; Wed, 3 May 2023 11:41:10 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4zSFDIZT2pUFIsMOx/dhT7l3r3UIPpSKNlbmDYYppzXgLNijoK45L0g4DFz3So+PycdMaV X-Received: by 2002:a05:6a21:3a86:b0:eb:e22b:efa6 with SMTP id zv6-20020a056a213a8600b000ebe22befa6mr23763778pzb.51.1683139269823; Wed, 03 May 2023 11:41:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683139269; cv=none; d=google.com; s=arc-20160816; b=YlaW8zXB/5ArqDcfbceaJoptKu+oorHkD5nWshf5IsR8tBr1E6r5RNvBJIAVm2b07Z MVwiTEt+Htj351v7zhB4XFVEzPUkgevETcV+cc8EDMN/hyMR7VRi9FCGkfxiumSatnKU n8aCl6W4q1JLuTSeTtOiGJ49upjMLiQqFYg7fEv7jO5QGTbr+xV1tBcLMTVGvM2nUkvt Pooj8rp2FQnf1f+e8LyKB5dinIhVedO72SnIS6isVAMdWYxYDzP4KdEZ6jq0U/yqxvkG 0HtQ9snp4B6ei8GNd9dy6tGMNF7NPapXdJFOyBJ8JGObbxQHkQOOMzZLF6XWA3YRl9sn QBwQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:reply-to:dkim-signature; bh=HR9sDhe1dBWwYsQO66isTNLen+QloTOYlqac4Dib2S0=; b=ycUiK8CTm4iY16cjQEHvE/2AOiDpqf7stzzgQfjGiW5a1JUJjR//+8846pLIjjWali K1VakEyjns1z4vV/1TUhkp1nbk5yYVV9G33kpKAHDLuM0NoI70Ghz4fzI4oYkFArZf0x qjX4aQXhEUJ0Vq9UHBjyMR29h0Z1wJcIwAYlG4KhWH+FfKw558Emw+hV/T6psCG/sSf9 ua/1mRBRKezrHkJ9cJpicPzQc0Tr/1tf7eLp8BQWuWBXJfIgAwjWH8O6bbLGVdS+mtvc JC/w6flE3vPkzOeZdyesQKhPYjFOZSyYlKHRRP3q9F25mW7M8fSzNbdEH1M7HntHNeK9 Bwag== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=RpPim3ur; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c12-20020aa7952c000000b0063cf9718a24si33283019pfp.388.2023.05.03.11.40.53; Wed, 03 May 2023 11:41:09 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=RpPim3ur; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229997AbjECS3E (ORCPT <rfc822;lhua1029@gmail.com> + 99 others); Wed, 3 May 2023 14:29:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60944 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229878AbjECS3B (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 3 May 2023 14:29:01 -0400 Received: from mail-pf1-x449.google.com (mail-pf1-x449.google.com [IPv6:2607:f8b0:4864:20::449]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2C64D35B6 for <linux-kernel@vger.kernel.org>; Wed, 3 May 2023 11:29:00 -0700 (PDT) Received: by mail-pf1-x449.google.com with SMTP id d2e1a72fcca58-64115ef7234so4721162b3a.1 for <linux-kernel@vger.kernel.org>; Wed, 03 May 2023 11:29:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683138539; x=1685730539; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:from:to:cc:subject:date:message-id:reply-to; bh=HR9sDhe1dBWwYsQO66isTNLen+QloTOYlqac4Dib2S0=; b=RpPim3urdDOUWuJ3wzy0utodCmxX0rjPl4mUUzwsMX1hw3s9PCKBeD92U/Flg1VcXo BMbqTLLdj3Gx+nc7TzpBr5JLhosnACTWbU/0TivFrLub70MDUpfAHs0InFAoISRRSUp5 4NlW5AgbyWlxTr1gGf8HlzJWL6KgxFkdynlz8+tHNMBJJlw7ATdALt5US3SEahkdNYd1 dIwiZLYlUblywLajLjSm/9axWRGxsCEhBzqFau67BCzEzwvURmB17++fg+byQc22wiYq iMsyTypPoYLBkHF5sXZ4TQC3Zsjq1YTb/kirR7uPmKL81tsozyRZG93BvGjGArcBuy/8 xDtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683138539; x=1685730539; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=HR9sDhe1dBWwYsQO66isTNLen+QloTOYlqac4Dib2S0=; b=JBFwPtBtzkgSrYUFLMT7pWEo7wi+ijo4daBR32oLbgOriXQ/cJX4tFwk+xZXt5wGeR ywqdL46Q66OFKvJwjA2WNoqGfH7+kk56Ix9CNAsqwuDeT4JAFcBGrqHVazHsm/8kDmNa kdet7Snxljj60bjEOZUjcPHux8gWciAX5YOfNloBWY3AHUngaQvL8XBxkUvbPnbpNgrq Lr9rGMYBSkyheNNH9+QP3ZJrYcizkRv05qz/QCvpITguLncmKlgvlwxOvtaWNQLTqlmr 69828F3F/YbMCm5UyO6t26Ifu9fY+gqtbe/UCMd8S1BGtfIls0uM2sttH1UmXTkXYSzM Yg6Q== X-Gm-Message-State: AC+VfDxHVsf37k2V2LUAwgnsWmyZumShl/8Q2Uhy9s56AnDFie4QENEs r8h+DBZXCTiaeVtmfSF6RexRzk08jFs= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a63:2c14:0:b0:51a:7d6a:65c9 with SMTP id s20-20020a632c14000000b0051a7d6a65c9mr714454pgs.6.1683138539464; Wed, 03 May 2023 11:28:59 -0700 (PDT) Reply-To: Sean Christopherson <seanjc@google.com> Date: Wed, 3 May 2023 11:28:48 -0700 In-Reply-To: <20230503182852.3431281-1-seanjc@google.com> Mime-Version: 1.0 References: <20230503182852.3431281-1-seanjc@google.com> X-Mailer: git-send-email 2.40.1.495.gc816e09b53d-goog Message-ID: <20230503182852.3431281-2-seanjc@google.com> Subject: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler From: Sean Christopherson <seanjc@google.com> To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Wenyao Hai <haiwenyao@uniontech.com>, Ke Guo <guoke@uniontech.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764899442866252114?= X-GMAIL-MSGID: =?utf-8?q?1764899442866252114?= |
Series |
KVM: x86: Clean up MSR PAT handling
|
|
Commit Message
Sean Christopherson
May 3, 2023, 6:28 p.m. UTC
From: Wenyao Hai <haiwenyao@uniontech.com> Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by case 0x200 ... MSR_IA32_MC0_CTL2 - 1: in kvm_set_msr_common(). Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com> [sean: massage changelog] Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Comments
On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote: > From: Wenyao Hai <haiwenyao@uniontech.com> > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). What's the value of doing so, besides saving a function of kvm_set_msr_common()? PAT change shouldn't be something frequent so shouldn't in a performance critical path. Given the PAT logic on Intel and AMD are basically the same , isn't it better to do in kvm_set_msr_common()? For instance, given mtrr code is also in common x86, if we ever want to add some additional logic to, i.e. calculate effective memtype, isn't better to do handle pat in common code too? > > Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by > > case 0x200 ... MSR_IA32_MC0_CTL2 - 1: > > in kvm_set_msr_common(). > > Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com> > [sean: massage changelog] > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 44fb619803b8..53e249109483 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT) > get_vmcs12(vcpu)->guest_ia32_pat = data; > > - if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > + if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > vmcs_write64(GUEST_IA32_PAT, data); > - vcpu->arch.pat = data; > - break; > - } > - ret = kvm_set_msr_common(vcpu, msr_info); > + > + vcpu->arch.pat = data; > break; > case MSR_IA32_MCG_EXT_CTL: > if ((!msr_info->host_initiated &&
On Wed, May 03, 2023, Kai Huang wrote: > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote: > > From: Wenyao Hai <haiwenyao@uniontech.com> > > > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). > > What's the value of doing so, besides saving a function of kvm_set_msr_common()? To avoid complicating a very simple operation (writing vcpu->arch.pat), and to align with SVM. > PAT change shouldn't be something frequent so shouldn't in a performance > critical path. Given the PAT logic on Intel and AMD are basically the same , > isn't it better to do in kvm_set_msr_common()? I could go either way on calling into kvm_set_msr_common(). I agree that performance isn't a concern. Hmm, and kvm_set_msr_common() still has a case statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't impact the code generation for other MSRs. Though I am leaning towards saying we should either handle loads and stores to vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to handle reads of PAT, or have their write paths call kvm_set_msr_common(). A mix of both is definitely odd. I don't have strong preference on which of those two we choose. I dislike duplicating logic across VMX and SVM, but on the other hands it's so little code. I think I'd vote for handling everything in vendor code, mostly because this gives the appearance that the write can fail, which is silly and misleading. ret = kvm_set_msr_common(vcpu, msr_info); > For instance, given mtrr code is also in common x86, if we ever want to add some > additional logic to, i.e. calculate effective memtype, isn't better to do handle > pat in common code too? FWIW, I highly doubt we'll ever have code like that. The truly effective memtype calculations are too different between Intel and AMD, and doing anything useful with the guest's effective memtype is likely a fool's errand. > > Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by > > > > case 0x200 ... MSR_IA32_MC0_CTL2 - 1: > > > > in kvm_set_msr_common(). > > > > Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com> > > [sean: massage changelog] > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 44fb619803b8..53e249109483 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT) > > get_vmcs12(vcpu)->guest_ia32_pat = data; > > > > - if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > + if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > > vmcs_write64(GUEST_IA32_PAT, data); > > - vcpu->arch.pat = data; > > - break; > > - } > > - ret = kvm_set_msr_common(vcpu, msr_info); > > + > > + vcpu->arch.pat = data; > > break; > > case MSR_IA32_MCG_EXT_CTL: > > if ((!msr_info->host_initiated &&
On Wed, 2023-05-03 at 16:25 -0700, Sean Christopherson wrote: > On Wed, May 03, 2023, Kai Huang wrote: > > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote: > > > From: Wenyao Hai <haiwenyao@uniontech.com> > > > > > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing > > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). > > > > What's the value of doing so, besides saving a function of kvm_set_msr_common()? > > To avoid complicating a very simple operation (writing vcpu->arch.pat), and to > align with SVM. > > > PAT change shouldn't be something frequent so shouldn't in a performance > > critical path. Given the PAT logic on Intel and AMD are basically the same , > > isn't it better to do in kvm_set_msr_common()? > > I could go either way on calling into kvm_set_msr_common(). I agree that > performance isn't a concern. Hmm, and kvm_set_msr_common() still has a case > statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't > impact the code generation for other MSRs. > > Though I am leaning towards saying we should either handle loads and stores to > vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to > handle reads of PAT, or have their write paths call kvm_set_msr_common(). A mix > of both is definitely odd. Agreed. Alternatively we can move SVM's setting vcpu->arch.pat to common code. > > I don't have strong preference on which of those two we choose. I dislike duplicating > logic across VMX and SVM, but on the other hands it's so little code. I think > I'd vote for handling everything in vendor code, mostly because this gives the > appearance that the write can fail, which is silly and misleading. > > ret = kvm_set_msr_common(vcpu, msr_info); No opinion either. First glance is having case MSR_IA32_CR_PAT: vcpu->arch.pat = data; in kvm_set_msr_common() is clearer because it is symmetrical to the read path. Anyway your decision :) > > > For instance, given mtrr code is also in common x86, if we ever want to add some > > additional logic to, i.e. calculate effective memtype, isn't better to do handle > > pat in common code too? > > FWIW, I highly doubt we'll ever have code like that. The truly effective memtype > calculations are too different between Intel and AMD, and doing anything useful > with the guest's effective memtype is likely a fool's errand. I thought the logic of getting effective memtype should be just the same between Intel and AMD but it seems there's slight difference. I agree with you it's unlikely to have such code in common. But looks setting vcpu->arch.pat in common code is more flexible. Anyway no opinion here. > > > > Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by > > > > > > case 0x200 ... MSR_IA32_MC0_CTL2 - 1: > > > > > > in kvm_set_msr_common(). > > > > > > Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com> > > > [sean: massage changelog] > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/vmx/vmx.c | 8 +++----- > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > index 44fb619803b8..53e249109483 100644 > > > --- a/arch/x86/kvm/vmx/vmx.c > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT) > > > get_vmcs12(vcpu)->guest_ia32_pat = data; > > > > > > - if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > > + if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > > > vmcs_write64(GUEST_IA32_PAT, data); > > > - vcpu->arch.pat = data; > > > - break; > > > - } > > > - ret = kvm_set_msr_common(vcpu, msr_info); > > > + > > > + vcpu->arch.pat = data; > > > break; > > > case MSR_IA32_MCG_EXT_CTL: > > > if ((!msr_info->host_initiated &&
On Wed, May 03, 2023, Kai Huang wrote: > On Wed, 2023-05-03 at 16:25 -0700, Sean Christopherson wrote: > > On Wed, May 03, 2023, Kai Huang wrote: > > > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote: > > > > From: Wenyao Hai <haiwenyao@uniontech.com> > > > > > > > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing > > > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). > > > > > > What's the value of doing so, besides saving a function of kvm_set_msr_common()? > > > > To avoid complicating a very simple operation (writing vcpu->arch.pat), and to > > align with SVM. > > > > > PAT change shouldn't be something frequent so shouldn't in a performance > > > critical path. Given the PAT logic on Intel and AMD are basically the same , > > > isn't it better to do in kvm_set_msr_common()? > > > > I could go either way on calling into kvm_set_msr_common(). I agree that > > performance isn't a concern. Hmm, and kvm_set_msr_common() still has a case > > statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't > > impact the code generation for other MSRs. > > > > Though I am leaning towards saying we should either handle loads and stores to > > vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to > > handle reads of PAT, or have their write paths call kvm_set_msr_common(). A mix > > of both is definitely odd. > > Agreed. Alternatively we can move SVM's setting vcpu->arch.pat to common code. > > > > > I don't have strong preference on which of those two we choose. I dislike duplicating > > logic across VMX and SVM, but on the other hands it's so little code. I think > > I'd vote for handling everything in vendor code, mostly because this gives the > > appearance that the write can fail, which is silly and misleading. > > > > ret = kvm_set_msr_common(vcpu, msr_info); > > No opinion either. First glance is having > > case MSR_IA32_CR_PAT: > vcpu->arch.pat = data; > > in kvm_set_msr_common() is clearer because it is symmetrical to the read path. > > Anyway your decision :) Duh, the obvious answer is to do ret = kvm_set_msr_common(vcpu, msr_info); if (ret) break; <vendor code here> That's an established pattern for other MSRs, and addresses my main concern of not unwinding the VMCS updates in the should-be-impossible scenario of kvm_set_msr_common() failing after the kvm_pat_valid() check. Thanks Kai!
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 44fb619803b8..53e249109483 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT) get_vmcs12(vcpu)->guest_ia32_pat = data; - if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { + if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) vmcs_write64(GUEST_IA32_PAT, data); - vcpu->arch.pat = data; - break; - } - ret = kvm_set_msr_common(vcpu, msr_info); + + vcpu->arch.pat = data; break; case MSR_IA32_MCG_EXT_CTL: if ((!msr_info->host_initiated &&