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

[Jan Beulich <jbeulich@suse.com>]

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@citrix.com>
>> 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. Whereas my patch switches to using the raw
policy, simply to eliminate redundant data. And your patch here is about
collecting raw policy earlier, too, for that to become usable by
xstate_init(). Differences between your any my variant are when exactly
raw policy collection happens, and that mine _additionally_ calculates
host policy a first time right after having calculated the raw one. My
patch specifically does not use the host policy in xstate_init(), nor in
the two new macros that are being introduced.

In the end it sounds like all you object to is my patch calculating the
host policy (a first time) earlier, too. As the description there says,
a subsequent change in that series needs this movement anyway. If some
suitable replacement for that dependency exists, I'm sure that early
calculation could be left out of the patch referenced above, if that's
indeed the sole concern.

>>> --- a/xen/arch/x86/cpu-policy.c
>>> +++ b/xen/arch/x86/cpu-policy.c
>>> @@ -845,7 +845,6 @@ static void __init calculate_hvm_def_policy(void)
>>>  
>>>  void __init init_guest_cpu_policies(void)
>>>  {
>>> -    calculate_raw_cpu_policy();
>>>      calculate_host_policy();
>>>  
>>>      if ( IS_ENABLED(CONFIG_PV) )
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1888,7 +1888,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>>>  
>>>      tsx_init(); /* Needs microcode.  May change HLE/RTM feature bits. */
>>>  
>>> -    identify_cpu(&boot_cpu_data);
>>> +    calculate_raw_cpu_policy(); /* Needs microcode.  No other dependenices. */
>>> +
>>> +    identify_cpu(&boot_cpu_data); /* Needs microcode and raw policy. */
>> You don't introduce any dependency on raw policy here, and there cannot possibly
>> have been such a dependency before (unless there was a bug somewhere). Therefore
>> I consider this latter comment misleading at this point.
> 
> It's made true by the next patch, and I'm not included to split the
> comment across two patches which are going to be committed together in a
> unit.

Which is fine, so long as this is then not done silently, leaving it to
reviewers to notice (or not). IOW please: Just mention anomalies like this
in half a sentence in the description. (Committing as a unit is also an
uncertain thing, as long as that's not put forth as a strict requirement
somewhere. We do partial commits of series all the time, after all.)

Jan