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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

This is necessary for the forthcoming xstate_{un,}compressed_size() to perform
boot-time sanity checks on state components which aren't fully enabled yet.  I
decided that doing this was better than extending the xstate_{offsets,sizes}[]
logic that we're intending to retire in due course.

v3:
 * New.
---
 xen/arch/x86/cpu-policy.c | 1 -
 xen/arch/x86/setup.c      | 4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index b96f4ee55cc4..5b66f002df05 100644
--- 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) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b50c9c84af6d..8850e5637a98 100644
--- 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. */
 
     set_in_cr4(X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT);
 
-- 
2.30.2

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.)

> --- 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.

Jan

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.

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.

xsave.c really has no legitimate use for the host policy once the
uniform allocation size aspect has gone away.


>> --- 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.

~Andrew

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

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@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.

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

On 17.06.2024 19:30, Andrew Cooper wrote:
> 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@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.
> 
> 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)

That's "x86/xstate: replace xsave_cntxt_size and drop XCNTXT_MASK", but
what I referenced in the context here is "x86/xstate: drop
xstate_offsets[] and xstate_sizes[]". If there use of the host policy
is wrong in your opinion, then can you please reply there to clarify
what's wrong with the justification for doing so? (See also below.)

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

Well, it's not quite "simply on its age". Part of me thinks that our
rules should permit for things to go in when no-one looked at them
despite reminders, but the other part of me knows that then we'll move
back to what we had in the old days, where all kinds of (subtle or
supposedly obvious) breakage would be introduced. So it's more like
"considering its age, it really should have had replies". Anyway, there
was now a vague promise that this series is going to be helped make
progress in the 4.20 cycle.

> 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.

That's a fair concern. My counter argument is that right now the host
policy can't possibly be used ahead of when its calculated prior to
that patch (i.e. ahead of the 2nd calculation after the patch). Yes,
care will need to be taken (for the time being) to, between the two
points, only access parts which can't change again during the 2nd
calculation.

Considering the plans to invoke identify_cpu() earlier, it simply didn't
appear necessary to, transiently, change setup_{force,clear}_cap().

> 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.

Again I'm confused, I'm afraid: xsave_cntxt_size isn't really used for
much guest-ish right now; its main use is in xstate_init(). Hence why
that other patch is replacing it by use of the host policy. Perhaps
you're simply suggesting that its present uses aren't quite right then;
as you say, that sole truly guest related use in xstate_alloc_save_area()
is there only because right now we need to over-estimate the eventual
size needed. For that purpose, host policy is the right thing to use, imo.
xsave_cntxt_size going away altogether at some point is no reason to,
transiently, switch it from being an actual variable to referencing the
host policy. It's just that if it can be made go away earlier, that part
of that other patch could then simply be dropped.

Jan