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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7DD31C433F5 for ; Tue, 28 Sep 2021 18:50:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5A78B60ED7 for ; Tue, 28 Sep 2021 18:50:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242223AbhI1SwM (ORCPT ); Tue, 28 Sep 2021 14:52:12 -0400 Received: from mga12.intel.com ([192.55.52.136]:29085 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230109AbhI1SwK (ORCPT ); Tue, 28 Sep 2021 14:52:10 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10121"; a="204269370" X-IronPort-AV: E=Sophos;i="5.85,330,1624345200"; d="scan'208";a="204269370" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Sep 2021 11:50:29 -0700 X-IronPort-AV: E=Sophos;i="5.85,330,1624345200"; d="scan'208";a="519215652" Received: from agluck-desk2.sc.intel.com (HELO agluck-desk2.amr.corp.intel.com) ([10.3.52.146]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Sep 2021 11:50:29 -0700 Date: Tue, 28 Sep 2021 11:50:26 -0700 From: "Luck, Tony" To: Dave Hansen Cc: Andy Lutomirski , Fenghua Yu , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "Peter Zijlstra (Intel)" , Lu Baolu , Joerg Roedel , Josh Poimboeuf , Dave Jiang , Jacob Jun Pan , Raj Ashok , "Shankar, Ravi V" , iommu@lists.linux-foundation.org, the arch/x86 maintainers , Linux Kernel Mailing List Subject: Re: [PATCH 4/8] x86/traps: Demand-populate PASID MSR via #GP Message-ID: References: <20210920192349.2602141-1-fenghua.yu@intel.com> <20210920192349.2602141-5-fenghua.yu@intel.com> <1aae375d-3cd4-4ab8-9c64-9e387916e6c0@www.fastmail.com> <035290e6-d914-a113-ea6c-e845d71069cf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <035290e6-d914-a113-ea6c-e845d71069cf@intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 27, 2021 at 04:51:25PM -0700, Dave Hansen wrote: > That's close to what we want. > > The size should probably be implicit. If it isn't implicit, it needs to > at least be double-checked against the state sizes. > > Not to get too fancy, but I think we also want to have a "replace" > operation which is separate from the "update". Think of a case where > you are trying to set a bit: > > struct pkru_state *pk = start_update_xstate(tsk, XSTATE_PKRU); > pk->pkru |= 0x100; > finish_update_xstate(tsk, XSTATE_PKRU, pk); > > versus setting a whole new value: > > struct pkru_state *pk = start_replace_xstate(tsk, XSTATE_PKRU); > memset(pkru, sizeof(*pk), 0); > pk->pkru = 0x1234; > finish_replace_xstate(tsk, XSTATE_PKRU, pk); > > They look similar, but they actually might have very different costs for > big states like AMX. We can also do some very different debugging for > them. In normal operation, these _can_ just return pointers directly to > the fpu...->xstate in some case. But, if we're debugging, we could > kmalloc() a buffer and do sanity checking before it goes into the task > buffer. > > We don't have to do any of that fancy stuff now. We don't even need to > do an "update" if all we need for now for XFEATURE_PASID is a "replace". > > 1. Hide whether we need to write to real registers > 2. Hide whether we need to update the in-memory image > 3. Hide other FPU infrastructure like the TIF flag. > 4. Make the users deal with a *whole* state in the replace API Is that difference just whether you need to save the state from registers to memory (for the "update" case) or not (for the "replace" case ... where you can ignore the current register, overwrite the whole per-feature xsave area and mark it to be restored to registers). If so, just a "bool full" argument might do the trick? Also - you have a "tsk" argument in your pseudo code. Is this needed? Are there places where we need to perform these operations on something other than "current"? pseudo-code: void *begin_update_one_xsave_feature(enum xfeature xfeature, bool full) { void *addr; BUG_ON(!(xsave->header.xcomp_bv & xfeature)); addr = __raw_xsave_addr(xsave, xfeature); fpregs_lock(); if (full) return addr; if (xfeature registers are "live") xsaves(xstate, 1 << xfeature); return addr; } void finish_update_one_xsave_feature(enum xfeature xfeature) { mark feature modified set TIF bit fpregs_unlock(); } -Tony