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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A4E4EC433EF for ; Sat, 12 Feb 2022 00:27:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 32FB76B007B; Fri, 11 Feb 2022 19:27:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E1D78D0001; Fri, 11 Feb 2022 19:27:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 159E26B007E; Fri, 11 Feb 2022 19:27:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0160.hostedemail.com [216.40.44.160]) by kanga.kvack.org (Postfix) with ESMTP id 08C7C6B007B for ; Fri, 11 Feb 2022 19:27:44 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C4CBF8248D52 for ; Sat, 12 Feb 2022 00:27:43 +0000 (UTC) X-FDA: 79132239606.25.D0830F8 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by imf15.hostedemail.com (Postfix) with ESMTP id A075DA0003 for ; Sat, 12 Feb 2022 00:27:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1644625662; x=1676161662; h=message-id:date:mime-version:to:references:from:subject: in-reply-to:content-transfer-encoding; bh=JnVsh9XoDVOYZL62w9WLXV6Np+XHCqYefzRHWFleKnU=; b=BtFbRaPu8AcdiOy9VkSytXCXxL/EAkr7kBHohBxUFkrfh3dmv+dwg53n ZLI8CrzHQc0Dfxgqnaeepe6qZ2rBfc/gaRMTh76YM7CgG8pa0rD30itbh oHigt+SrBzUHXQ8BUN0dz62Q+rhQeX0FT4Gqse0hlrvTL7xHbnzhGZn42 JyOlqxoUNSxN0gxolzTouKBoJu6GmQapuINLkgCu1V3ZiYfJ2ggzBW0f8 4xsPgWwA24kXaNOoYVGB8YK0Ojc7hl42czzmzYiuu/+cDnztdZ88KGzBz xwkkRqTWEc0IZ5H/B11KRPthiVxg44yffoxXEUBaybF9L/BciIvRG8mG6 g==; X-IronPort-AV: E=McAfee;i="6200,9189,10255"; a="233387791" X-IronPort-AV: E=Sophos;i="5.88,361,1635231600"; d="scan'208";a="233387791" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Feb 2022 16:27:41 -0800 X-IronPort-AV: E=Sophos;i="5.88,361,1635231600"; d="scan'208";a="542298930" Received: from nsmdimra-mobl.amr.corp.intel.com (HELO [10.209.96.127]) ([10.209.96.127]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Feb 2022 16:27:40 -0800 Message-ID: Date: Fri, 11 Feb 2022 16:27:37 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: en-US To: Rick Edgecombe , x86@kernel.org, "H . Peter Anvin" , Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, Arnd Bergmann , Andy Lutomirski , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H . J . Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V . Shankar" , Dave Martin , Weijiang Yang , "Kirill A . Shutemov" , joao.moreira@intel.com, John Allen , kcc@google.com, eranian@google.com References: <20220130211838.8382-1-rick.p.edgecombe@intel.com> <20220130211838.8382-24-rick.p.edgecombe@intel.com> From: Dave Hansen Subject: Re: [PATCH 23/35] x86/fpu: Add helpers for modifying supervisor xstate In-Reply-To: <20220130211838.8382-24-rick.p.edgecombe@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: A075DA0003 X-Rspam-User: Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BtFbRaPu; spf=none (imf15.hostedemail.com: domain of dave.hansen@intel.com has no SPF policy when checking 134.134.136.126) smtp.mailfrom=dave.hansen@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Stat-Signature: 66fhi5ybgppgeimrrhi1hxz4mffpz9fe X-HE-Tag: 1644625662-459199 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 1/30/22 13:18, Rick Edgecombe wrote: > Add helpers that can be used to modify supervisor xstate safely for the > current task. This should be at the end of the changelog. > State for supervisors xstate based features can be live and > accesses via MSR's, or saved in memory in an xsave buffer. When the > kernel needs to modify this state it needs to be sure to operate on it > in the right place, so the modifications don't get clobbered. We tend to call these "supervisor xfeatures". The "state is in the registers" we call "active". Maybe: Just like user xfeatures, supervisor xfeatures can be either active in the registers or inactive and present in the task FPU buffer. If the registers are active, the registers can be modified directly. If the registers are not active, the modification must be performed on the task FPU buffer. > In the past supervisor xstate features have used get_xsave_addr() > directly, and performed open coded logic handle operating on the saved > state correctly. This has posed two problems: > 1. It has logic that has been gotten wrong more than once. > 2. To reduce code, less common path's are not optimized. Determination "paths" ^ > xstate = start_update_xsave_msrs(XFEATURE_FOO); > r = xsave_rdmsrl(state, MSR_IA32_FOO_1, &val) > if (r) > xsave_wrmsrl(state, MSR_IA32_FOO_2, FOO_ENABLE); > end_update_xsave_msrs(); This looks OK. I'm not thrilled about it. The start_update_xsave_msrs() can probably drop the "_msrs". Maybe: start_xfeature_update(...); Also, if you have to do the address lookup in xsave_rdmsrl() anyway, I wonder if the 'xstate' should just be a full fledged 'struct xregs_state'. The other option would be to make a little on-stack structure like: struct xsave_update { int feature; struct xregs_state *xregs; }; Then you do: struct xsave_update xsu; ... start_update_xsave_msrs(&xsu, XFEATURE_FOO); and then pass it along to each of the other operations: r = xsave_rdmsrl(xsu, MSR_IA32_FOO_1, &val) It's slightly less likely to get type confused as a 'void *'; > +static u64 *__get_xsave_member(void *xstate, u32 msr) > +{ > + switch (msr) { > + /* Currently there are no MSR's supported */ > + default: > + WARN_ONCE(1, "x86/fpu: unsupported xstate msr (%u)\n", msr); > + return NULL; > + } > +} Just to get an idea what this is doing, it's OK to include the shadow stack MSRs in here. Are you sure this should return a u64*? We have lots of <=64-bit XSAVE fields. > +/* > + * Return a pointer to the xstate for the feature if it should be used, or NULL > + * if the MSRs should be written to directly. To do this safely, using the > + * associated read/write helpers is required. > + */ > +void *start_update_xsave_msrs(int xfeature_nr) > +{ > + void *xstate; > + > + /* > + * fpregs_lock() only disables preemption (mostly). So modifing state modifying ^ > + * in an interrupt could screw up some in progress fpregs operation, ^ in-progress > + * but appear to work. Warn about it. > + */ > + WARN_ON_ONCE(!in_task()); > + WARN_ON_ONCE(current->flags & PF_KTHREAD); This might also be a good spot to check that xfeature_nr is in fpstate.xfeatures. > + fpregs_lock(); > + > + fpregs_assert_state_consistent(); > + > + /* > + * If the registers don't need to be reloaded. Go ahead and operate on the > + * registers. > + */ > + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) > + return NULL; > + > + xstate = get_xsave_addr(¤t->thread.fpu.fpstate->regs.xsave, xfeature_nr); > + > + /* > + * If regs are in the init state, they can't be retrieved from > + * init_fpstate due to the init optimization, but are not nessarily necessarily ^ Spell checker time. ":set spell" in vim works for me nicely. > + * zero. The only option is to restore to make everything live and > + * operate on registers. This will clear TIF_NEED_FPU_LOAD. > + * > + * Otherwise, if not in the init state but TIF_NEED_FPU_LOAD is set, > + * operate on the buffer. The registers will be restored before going > + * to userspace in any case, but the task might get preempted before > + * then, so this possibly saves an xsave. > + */ > + if (!xstate) > + fpregs_restore_userregs(); Won't fpregs_restore_userregs() end up setting TIF_NEED_FPU_LOAD=0? Isn't that a case where a "return NULL" is needed? In any case, this makes me think this code should start out stupid and slow. Keep the API as-is, but make the first patch unconditionally do the WRMSR. Leave the "fast" buffer modifications for a follow-on patch.