From: Dave Hansen <dave.hansen@intel.com>
To: "Chang S. Bae" <chang.seok.bae@intel.com>,
bp@suse.de, luto@kernel.org, tglx@linutronix.de,
mingo@kernel.org, x86@kernel.org
Cc: len.brown@intel.com, thiago.macieira@intel.com,
jing2.liu@intel.com, ravi.v.shankar@intel.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 08/26] x86/fpu/xstate: Introduce helpers to manage the XSTATE buffer dynamically
Date: Mon, 30 Aug 2021 10:45:08 -0700 [thread overview]
Message-ID: <bb49fdc9-2228-8bd1-bcc5-7c498daf0887@intel.com> (raw)
In-Reply-To: <20210730145957.7927-9-chang.seok.bae@intel.com>
On 7/30/21 7:59 AM, Chang S. Bae wrote:
> +/**
> + * get_xstate_size - Calculate an xstate buffer size
Calculate the amount of space needed to store an xstate buffer with the
given features.
> + * @mask: This bitmap tells which components reserved in the buffer.
The set of components for which the space is needed.
> + * Available once those arrays for the offset, size, and alignment info are
> + * set up, by setup_xstate_features().
Please just say:
Consults values populated in setup_xstate_features(). Must be
called after that setup.
> + * Returns: The buffer size
> + */
> +unsigned int get_xstate_size(u64 mask)
> +{
> + unsigned int size;
> + int i, nr;
> +
> + if (!mask)
> + return 0;
> +
> + /*
> + * The minimum buffer size excludes the dynamic user state. When a
> + * task uses the state, the buffer can grow up to the max size.
> + */
> + if (mask == (xfeatures_mask_all & ~xfeatures_mask_user_dynamic))
> + return get_xstate_config(XSTATE_MIN_SIZE);
> + else if (mask == xfeatures_mask_all)
> + return get_xstate_config(XSTATE_MAX_SIZE);
Is this just an optimization? It seems redundant with everything below.
I think that adds to the confusion.
> + nr = fls64(mask) - 1;
"nr" is a really, really, confusing name for this. "last_feature_nr"
might be better. Otherwise, this might be read as "number of features".
Comment might have helped, had there been any.
> + if (!boot_cpu_has(X86_FEATURE_XSAVES))
> + return xstate_offsets[nr] + xstate_sizes[nr];
Doesn't xstate_comp_offsets[] also work for non-compacted features?
setup_xstate_comp_offsets() says so and __raw_xsave_addr() depends on
that behavior.
> + if ((xfeatures_mask_all & (BIT_ULL(nr + 1) - 1)) == mask)
> + return xstate_comp_offsets[nr] + xstate_sizes[nr];
OK, so this is basically saying, "Is the size I'm looking for already
calculated and stored in xstate_comp_offsets[] because the mask is a
subset of xfeatures_mask_all". Right?
I guess that work. But, that's a *LOT* of logic to go uncommented.
> + /*
> + * With the given mask, no relevant size is found so far. So,
> + * calculate it by summing up each state size.
> + */
> + for (size = FXSAVE_SIZE + XSAVE_HDR_SIZE, i = FIRST_EXTENDED_XFEATURE; i <= nr; i++) {
> + if (!(mask & BIT_ULL(i)))
> + continue;
> +
> + if (xstate_aligns[i])
> + size = ALIGN(size, 64);
> + size += xstate_sizes[i];
> + }
> + return size;
> +}
OK, so this finally reveals something important about the function. It
is *trying* to avoid running this loop. All of the above is really just
optimizations to try and avoid doing this loop.
That makes me wonder why you chose that particular set of optimizations.
It also makes me wonder if they're even necessary.
So, first of all, why is this a new loop? Can't it share code with the
XSAVE setup code? That code also calculates the amount of space needed
for an XSAVE buffer given a mask.
Second, which of those optimizations do we *need*? I worry that this is
trying to be way too generic and be *optimized* for being generic code
when it will never really get random masks as input.
For instance, who is going to be calling this with
mask!=xfeatures_mask_all with !boot_cpu_has(X86_FEATURE_XSAVES)? That
seems rather improbable.
next prev parent reply other threads:[~2021-08-30 17:45 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 14:59 [PATCH v9 00/26] x86: Support Intel Advanced Matrix Extensions Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 01/26] x86/fpu/xstate: Modify the initialization helper to handle both static and dynamic buffers Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 02/26] x86/fpu/xstate: Modify state copy helpers " Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 03/26] x86/fpu/xstate: Modify address finders " Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 04/26] x86/fpu/xstate: Add a new variable to indicate dynamic user states Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 05/26] x86/fpu/xstate: Add new variables to indicate dynamic XSTATE buffer size Chang S. Bae
2021-08-12 15:03 ` Borislav Petkov
2021-07-30 14:59 ` [PATCH v9 06/26] x86/fpu/xstate: Calculate and remember dynamic XSTATE buffer sizes Chang S. Bae
2021-08-12 16:36 ` Borislav Petkov
2021-07-30 14:59 ` [PATCH v9 07/26] x86/fpu/xstate: Convert the struct fpu 'state' field to a pointer Chang S. Bae
2021-08-12 17:09 ` Borislav Petkov
2021-07-30 14:59 ` [PATCH v9 08/26] x86/fpu/xstate: Introduce helpers to manage the XSTATE buffer dynamically Chang S. Bae
2021-08-12 19:44 ` Borislav Petkov
2021-08-13 8:04 ` Bae, Chang Seok
2021-08-13 10:04 ` Borislav Petkov
2021-08-13 19:43 ` Bae, Chang Seok
2021-08-18 9:28 ` Borislav Petkov
2021-08-18 19:46 ` Bae, Chang Seok
2021-08-25 16:01 ` Bae, Chang Seok
2021-08-30 17:07 ` Borislav Petkov
2021-08-30 23:39 ` Bae, Chang Seok
2021-08-16 18:33 ` Bae, Chang Seok
2021-08-16 18:53 ` Borislav Petkov
2021-08-30 17:45 ` Dave Hansen [this message]
2021-08-30 23:39 ` Bae, Chang Seok
2021-07-30 14:59 ` [PATCH v9 09/26] x86/fpu/xstate: Update the XSTATE save function to support dynamic states Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 10/26] x86/fpu/xstate: Update the XSTATE buffer address finder " Chang S. Bae
2021-08-18 11:33 ` Borislav Petkov
2021-08-18 19:47 ` Bae, Chang Seok
2021-08-30 17:18 ` Borislav Petkov
2021-08-30 23:38 ` Bae, Chang Seok
2021-07-30 14:59 ` [PATCH v9 11/26] x86/fpu/xstate: Update the XSTATE context copy function " Chang S. Bae
2021-08-18 12:03 ` Borislav Petkov
2021-08-18 19:47 ` Bae, Chang Seok
2021-07-30 14:59 ` [PATCH v9 12/26] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state Chang S. Bae
2021-08-18 16:24 ` Borislav Petkov
2021-08-18 17:20 ` Thiago Macieira
2021-08-18 17:46 ` Borislav Petkov
2021-08-18 17:58 ` Thiago Macieira
2021-08-18 18:10 ` Borislav Petkov
2021-08-24 22:51 ` Len Brown
2021-08-18 20:43 ` Bae, Chang Seok
2021-08-18 21:04 ` Thiago Macieira
2021-08-18 21:12 ` Bae, Chang Seok
2021-08-18 22:27 ` Thiago Macieira
2021-08-19 1:21 ` Andy Lutomirski
2021-08-19 16:06 ` Thiago Macieira
2021-08-18 21:17 ` Borislav Petkov
2021-08-18 21:37 ` Bae, Chang Seok
2021-08-19 8:00 ` Borislav Petkov
2021-08-19 15:24 ` Bae, Chang Seok
2021-08-24 23:22 ` Len Brown
2021-08-30 17:31 ` Borislav Petkov
2021-09-17 3:48 ` Len Brown
2021-08-18 19:47 ` Bae, Chang Seok
2021-08-24 22:21 ` Len Brown
2021-08-30 17:41 ` Borislav Petkov
2021-08-31 21:44 ` Len Brown
2021-08-24 23:17 ` Len Brown
2021-08-30 17:53 ` Borislav Petkov
2021-08-31 22:07 ` Len Brown
2021-08-31 22:11 ` Dave Hansen
2021-08-30 18:04 ` Dave Hansen
2021-08-31 22:15 ` Len Brown
2021-08-31 22:16 ` Len Brown
2021-08-31 22:39 ` Thiago Macieira
2021-08-31 22:44 ` Len Brown
2021-07-30 14:59 ` [PATCH v9 13/26] x86/fpu/xstate: Support ptracer-induced XSTATE buffer expansion Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 14/26] x86/arch_prctl: Create ARCH_SET_STATE_ENABLE/ARCH_GET_STATE_ENABLE Chang S. Bae
2021-08-06 16:46 ` Thiago Macieira
2021-08-09 22:08 ` Bae, Chang Seok
2021-08-09 23:42 ` Thiago Macieira
2021-08-10 0:57 ` Bae, Chang Seok
2021-08-13 19:44 ` Bae, Chang Seok
2021-07-30 14:59 ` [PATCH v9 15/26] x86/fpu/xstate: Support both legacy and expanded signal XSTATE size Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 16/26] x86/fpu/xstate: Adjust the XSAVE feature table to address gaps in state component numbers Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 17/26] x86/fpu/xstate: Disable XSTATE support if an inconsistent state is detected Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 18/26] x86/cpufeatures/amx: Enumerate Advanced Matrix Extension (AMX) feature bits Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 19/26] x86/fpu/amx: Define AMX state components and have it used for boot-time checks Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 20/26] x86/fpu/amx: Initialize child's AMX state Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 21/26] x86/fpu/amx: Enable the AMX feature in 64-bit mode Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 22/26] x86/fpu/xstate: Skip writing zeros to signal frame for dynamic user states if in INIT-state Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 23/26] selftest/x86/amx: Test cases for the AMX state management Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 24/26] x86/insn/amx: Add TILERELEASE instruction to the opcode map Chang S. Bae
2021-07-30 14:59 ` [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA capability Chang S. Bae
2021-07-30 18:41 ` Dave Hansen
2021-08-03 21:32 ` Bae, Chang Seok
2021-08-03 21:38 ` Dave Hansen
2021-08-03 21:43 ` Brown, Len
2021-07-30 20:15 ` Dave Hansen
2021-07-30 14:59 ` [PATCH v9 26/26] x86/fpu/xstate: Add a sanity check for XFD state when saving XSTATE Chang S. Bae
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb49fdc9-2228-8bd1-bcc5-7c498daf0887@intel.com \
--to=dave.hansen@intel.com \
--cc=bp@suse.de \
--cc=chang.seok.bae@intel.com \
--cc=jing2.liu@intel.com \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@kernel.org \
--cc=ravi.v.shankar@intel.com \
--cc=tglx@linutronix.de \
--cc=thiago.macieira@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.