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 <jbeul...@suse.com>
Jan
