Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/06/2024 11:25 am, Jan Beulich wrote:
> On 14.06.2024 20:26, Andrew Cooper wrote:
>> On 23/05/2024 4:44 pm, Jan Beulich wrote:
>>> On 23.05.2024 13:16, Andrew Cooper wrote:
>>>> This is a tangle, but it's a small step in the right direction.
>>>>
>>>> xstate_init() is shortly going to want data from the Raw policy.
>>>> calculate_raw_cpu_policy() is sufficiently separate from the other 
>>>> policies to
>>>> be safe to do.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Would you mind taking a look at
>>> https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
>>> to make clear (to me at least) in how far we can perhaps find common grounds
>>> on what wants doing when? (Of course the local version I have has been
>>> constantly re-based, so some of the function names would have changed from
>>> what's visible there.)
>> That's been covered several times, at least in part.
>>
>> I want to eventually move the host policy too, but I'm not willing to
>> compound the mess we've currently got just to do it earlier.  It's just
>> creating even more obstacles to doing it nicely.
>>
>> Nothing in this series needs (or indeed should) use the host policy.
> Hmm, I'm irritated: You talk about host policy here, ...
>
>> The same is true of your AMX series.  You're (correctly) breaking the
>> uniform allocation size and (when policy selection is ordered WRT vCPU
>> creation, as discussed) it becomes solely depend on the guest policy.
> ... then guest policy, and ...
>
>> xsave.c really has no legitimate use for the host policy once the
>> uniform allocation size aspect has gone away.
> ... then host policy again.

Yes.  Notice how host policy is always referred to in the negative.

The raw policy should be used for everything pertaining to the
instruction ABI itself, and the guest policy for sizing information.

> Whereas my patch switches to using the raw
> policy, simply to eliminate redundant data.

Except it doesn't.  The latest posted version of your series contains:

-static u32 __read_mostly xsave_cntxt_size;
+#define xsave_cntxt_size (host_cpuid_policy.xstate.max_size | 0)

and you've even stated that I should have acked this patch simply on its
age.

I acked the patches that were good, and you did committed them at the
time.  Then I put together this series to fix the bugs the latent bugs
which you were making less latent; this series really is the same one
discussed back then, and really does have some 2020/2021 author dates in it.


It is, AFAICT, not safe to move the calculation of the host policy as
early as you did, without arranging for setup_{force,clear}_cap() to
edit the host policy synchronously.  Recalculating a second time later
isn't good enough, because you've created an asymmetry for most of boot
between two pieces of state which are supposed to be in sync, and that
you're intentionally starting to use.  So yes - while I do intend to
eventually make the host policy usable that early too, I'm really not
happy doing so in a manner that has "ticking timebomb" written all over it.

As to xsave_cntxt_size, it can (and should) be eliminated entirely when
xstate_alloc_save_area() can use the guest policy to size the allocation.

~Andrew