On 23.05.2024 13:16, Andrew Cooper wrote:
> @@ -611,6 +587,40 @@ static bool valid_xcr0(uint64_t xcr0)
> return true;
> }
>
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
> +{
> + unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
> + ASSERT((xcr0 & ~X86_XCR0_STATES) == 0);
I'm puzzled by the combination of this assertion and ...
> + if ( xcr0 == xfeature_mask )
> + return xsave_cntxt_size;
... this conditional return. Yes, right now we don't support/use any XSS
components, but without any comment the assertion looks overly restrictive
to me.
> @@ -818,14 +834,14 @@ void xstate_init(struct cpuinfo_x86 *c)
> * xsave_cntxt_size is the max size required by enabled features.
> * We know FP/SSE and YMM about eax, and nothing about edx at
> present.
> */
> - xsave_cntxt_size = hw_uncompressed_size(feature_mask);
> + xsave_cntxt_size = cpuid_count_ebx(0xd, 0);
> printk("xstate: size: %#x and states: %#"PRIx64"\n",
> xsave_cntxt_size, xfeature_mask);
> }
> else
> {
> BUG_ON(xfeature_mask != feature_mask);
> - BUG_ON(xsave_cntxt_size != hw_uncompressed_size(feature_mask));
> + BUG_ON(xsave_cntxt_size != cpuid_count_ebx(0xd, 0));
> }
Hmm, this may make re-basing of said earlier patch touching this code yet
more interesting. Or maybe it actually simplifies things, will need to see
... The overall comment remains though: Patches pending for so long should
really take priority over creating yet more new ones. But what do I do - I
can't enforce this, unless I was now going to block your work the same way.
Which I don't mean to do.
Jan
