| Message ID | 20240523111627.28896-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/xstate: Fixes to size calculations | expand |
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
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);
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(-)