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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EEF2FC4332B for ; Tue, 9 Mar 2021 05:32:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C70BA652BA for ; Tue, 9 Mar 2021 05:32:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229720AbhCIFcU (ORCPT ); Tue, 9 Mar 2021 00:32:20 -0500 Received: from mga07.intel.com ([134.134.136.100]:19796 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229577AbhCIFb7 (ORCPT ); Tue, 9 Mar 2021 00:31:59 -0500 IronPort-SDR: 7pZJoSZ1F7aVM1opUOgR0xP4cRYS0OCfT5keBAlKx+JkJ4XA49FsCAh7kgfSWrpXWrnWa77PXq RLGACKbbauzQ== X-IronPort-AV: E=McAfee;i="6000,8403,9917"; a="252188877" X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="252188877" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Mar 2021 21:31:58 -0800 IronPort-SDR: uf4phKz1TYjzv4cjOC+7I5P++yGKncold3EPtLN7RuVyTA48MxO9FBa0NGlxr5VFuH0/Vp1ZRE INovo7saS2qQ== X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="447377192" Received: from unknown (HELO [10.238.130.230]) ([10.238.130.230]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Mar 2021 21:31:54 -0800 Subject: Re: [PATCH v2] KVM: x86: Revise guest_fpu xcomp_bv field To: Sean Christopherson Cc: pbonzini@redhat.com, Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, "H. Peter Anvin" , kvm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20210225104955.3553-1-jing2.liu@linux.intel.com> From: "Liu, Jing2" Message-ID: Date: Tue, 9 Mar 2021 13:31:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/2/2021 7:59 AM, Sean Christopherson wrote: > On Thu, Feb 25, 2021, Jing Liu wrote: >> XCOMP_BV[63] field indicates that the save area is in the compacted >> format and XCOMP_BV[62:0] indicates the states that have space allocated >> in the save area, including both XCR0 and XSS bits enabled by the host >> kernel. Use xfeatures_mask_all for calculating xcomp_bv and reuse >> XCOMP_BV_COMPACTED_FORMAT defined by kernel. >> >> Signed-off-by: Jing Liu >> --- >> arch/x86/kvm/x86.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 1b404e4d7dd8..f115493f577d 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4435,8 +4435,6 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu, >> return 0; >> } >> >> -#define XSTATE_COMPACTION_ENABLED (1ULL << 63) >> - >> static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) >> { >> struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; >> @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src) >> /* Set XSTATE_BV and possibly XCOMP_BV. */ >> xsave->header.xfeatures = xstate_bv; >> if (boot_cpu_has(X86_FEATURE_XSAVES)) >> - xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED; >> + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | >> + xfeatures_mask_all; > Doesn't fill_xsave also need to be updated? Not with xfeatures_mask_all, but > to account for arch.ia32_xss? I believe it's a nop with the current code, since > supported_xss is zero, but it should be fixed, no? Yes. For the arch.ia32_xss, I noticed CET (https://lkml.org/lkml/2020/7/15/1412) has posted related change so I didn't touch xstate_bv for fill_xsave for now. Finally, fill_xsave() need e.g. arch.guest_supported_xss for xstate_bv, for xcomp_bv, xfeatures_mask_all is ok. > >> >> /* >> * Copy each region from the non-compacted offset to the >> @@ -9912,9 +9911,6 @@ static void fx_init(struct kvm_vcpu *vcpu) >> return; >> >> fpstate_init(&vcpu->arch.guest_fpu->state); >> - if (boot_cpu_has(X86_FEATURE_XSAVES)) >> - vcpu->arch.guest_fpu->state.xsave.header.xcomp_bv = >> - host_xcr0 | XSTATE_COMPACTION_ENABLED; > Ugh, this _really_ needs a comment in the changelog. It took me a while to > realize fpstate_init() does exactly what the new fill_xave() is doing. How about introducing that "fx_init()->fpstate_init() initializes xcomp_bv of guest_fpu so no need to set again in later fill_xsave() and load_xsave()" in commit message? > > And isn't the code in load_xsave() redundant and can be removed? Oh, yes. Keep fx_init() initializing xcomp_bv for guest_fpu is enough. Let me remove it in load_xsave later. And for fill_xsave(), I think no need to set xcomp_bv there. > Any code that > uses get_xsave_addr() would be have a dependency on load_xsave() if it's not > redundant, and I can't see how that would work. Sorry I didn't quite understand why get_xsave_addr() has dependency on load_xsave(), do you mean the xstate_bv instead of xcomp_bv, that load_xsave() uses it to get the addr? Thanks, Jing > >> >> /* >> * Ensure guest xcr0 is valid for loading >> -- >> 2.18.4 >>