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"?
> 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?
(For .xss there's no issue because XSTATE_XSAVES_ONLY is still 0.)
Jan
