From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate()
Date: Thu, 2 May 2024 16:33:57 +0200 [thread overview]
Message-ID: <dad023f2-c74b-4067-a3e3-4236d9424689@suse.com> (raw)
In-Reply-To: <b5560120-96c5-4b97-a4cc-36fbcfdd223c@citrix.com>
On 02.05.2024 15:24, Andrew Cooper wrote:
> On 02/05/2024 1:39 pm, Jan Beulich wrote:
>> On 29.04.2024 20:28, Andrew Cooper wrote:
>>> Make use of the new xstate_uncompressed_size() helper rather than maintaining
>>> the running calculation while accumulating feature components.
>> xstate_uncompressed_size() isn't really a new function, but the re-work of
>> an earlier one. That, aiui, could have been used here, too, just that it
>> would have been inefficient to do so. IOW perhaps drop "the new"?
>
> Ok.
>
>>
>>> The rest of the CPUID data can come direct from the raw cpu policy. All
>>> per-component data form an ABI through the behaviour of the X{SAVE,RSTOR}*
>>> instructions.
>>>
>>> Use for_each_set_bit() rather than opencoding a slightly awkward version of
>>> it. Mask the attributes in ecx down based on the visible features. This
>>> isn't actually necessary for any components or attributes defined at the time
>>> of writing (up to AMX), but is added out of an abundance of caution.
>> As to this, ...
>>
>>> @@ -206,61 +205,47 @@ static void recalculate_xstate(struct cpu_policy *p)
>>> return;
>>>
>>> if ( p->basic.avx )
>>> - {
>>> xstates |= X86_XCR0_YMM;
>>> - xstate_size = max(xstate_size,
>>> - xstate_offsets[X86_XCR0_YMM_POS] +
>>> - xstate_sizes[X86_XCR0_YMM_POS]);
>>> - }
>>>
>>> if ( p->feat.mpx )
>>> - {
>>> xstates |= X86_XCR0_BNDREGS | X86_XCR0_BNDCSR;
>>> - xstate_size = max(xstate_size,
>>> - xstate_offsets[X86_XCR0_BNDCSR_POS] +
>>> - xstate_sizes[X86_XCR0_BNDCSR_POS]);
>>> - }
>>>
>>> if ( p->feat.avx512f )
>>> - {
>>> xstates |= X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM;
>>> - xstate_size = max(xstate_size,
>>> - xstate_offsets[X86_XCR0_HI_ZMM_POS] +
>>> - xstate_sizes[X86_XCR0_HI_ZMM_POS]);
>>> - }
>>>
>>> if ( p->feat.pku )
>>> - {
>>> xstates |= X86_XCR0_PKRU;
>>> - xstate_size = max(xstate_size,
>>> - xstate_offsets[X86_XCR0_PKRU_POS] +
>>> - xstate_sizes[X86_XCR0_PKRU_POS]);
>>> - }
>>>
>>> - p->xstate.max_size = xstate_size;
>>> + /* Subleaf 0 */
>>> + p->xstate.max_size =
>>> + xstate_uncompressed_size(xstates & ~XSTATE_XSAVES_ONLY);
>>> p->xstate.xcr0_low = xstates & ~XSTATE_XSAVES_ONLY;
>>> p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
>>>
>>> + /* Subleaf 1 */
>>> p->xstate.Da1 = Da1;
>>> + if ( p->xstate.xsavec )
>>> + ecx_mask |= XSTATE_ALIGN64;
>>> +
>>> if ( p->xstate.xsaves )
>>> {
>>> + ecx_mask |= XSTATE_XSS;
>>> p->xstate.xss_low = xstates & XSTATE_XSAVES_ONLY;
>>> p->xstate.xss_high = (xstates & XSTATE_XSAVES_ONLY) >> 32;
>>> }
>>> - else
>>> - xstates &= ~XSTATE_XSAVES_ONLY;
>>>
>>> - for ( i = 2; i < min(63UL, ARRAY_SIZE(p->xstate.comp)); ++i )
>>> + /* Subleafs 2+ */
>>> + xstates &= ~XSTATE_FP_SSE;
>>> + BUILD_BUG_ON(ARRAY_SIZE(p->xstate.comp) < 63);
>>> + for_each_set_bit ( i, &xstates, 63 )
>>> {
>>> - uint64_t curr_xstate = 1UL << i;
>>> -
>>> - if ( !(xstates & curr_xstate) )
>>> - continue;
>>> -
>>> - p->xstate.comp[i].size = xstate_sizes[i];
>>> - p->xstate.comp[i].offset = xstate_offsets[i];
>>> - p->xstate.comp[i].xss = curr_xstate & XSTATE_XSAVES_ONLY;
>>> - p->xstate.comp[i].align = curr_xstate & xstate_align;
>> ... for this bit, isn't the move from this ...
>>
>>> + /*
>>> + * Pass through size (eax) and offset (ebx) directly. Visbility of
>>> + * attributes in ecx limited by visible features in Da1.
>>> + */
>>> + p->xstate.raw[i].a = raw_cpu_policy.xstate.raw[i].a;
>>> + p->xstate.raw[i].b = raw_cpu_policy.xstate.raw[i].b;
>>> + p->xstate.raw[i].c = raw_cpu_policy.xstate.raw[i].c & ecx_mask;
>> ... to this changing what guests get to see, i.e. (mildly?) incompatible?
>
> No.
>
> The only "rows" in leaf 0xd we expose to guests are AVX, MPX, AVX512 and
> PKU (higher up in this hunk, selecting valid bits in xstates). None of
> these have a non-zero value in ecx.
>
> This is a latent bug until we offer AMX or CET, hence why I wanted to
> complete this series before your AMX series goes in.
Oh, so finally a description of that very issue you kept mentioning. With
the small tweak to the description then
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
next prev parent reply other threads:[~2024-05-02 14:34 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
2024-05-02 12:08 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
2024-05-02 12:19 ` Jan Beulich
2024-05-02 13:17 ` Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
2024-05-02 12:39 ` Jan Beulich
2024-05-02 13:24 ` Andrew Cooper
2024-05-02 14:33 ` Jan Beulich [this message]
2024-04-29 18:28 ` [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves Andrew Cooper
2024-05-02 13:04 ` Jan Beulich
2024-05-02 14:32 ` Andrew Cooper
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=dad023f2-c74b-4067-a3e3-4236d9424689@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=xen-devel@lists.xenproject.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).