| Message ID | b789679a7edd41c88eca41d3c703d2292cfcce0e.1723806405.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/CPU: optional build of Intel/AMD CPUs support | expand |
On 16.08.2024 13:14, Sergiy Kibrik wrote: > Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when > corresponding CPU support is disabled by configuration less dead code will end > up in the build. > > This includes re-ordering of calls to ibpb_calculations() & div_calculations(), > but since they don't access common variables or feature bits it should be > safe to do. > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > CC: Jan Beulich <jbeulich@suse.com> For one please consider adding Requested-by: or Suggested-by: tags. > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -1012,6 +1012,7 @@ static bool __init should_use_eager_fpu(void) > } > } > > +#ifdef CONFIG_AMD > /* > * https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf > */ > @@ -1110,6 +1111,7 @@ static void __init div_calculations(bool hw_smt_enabled) > "enabled. Please assess your configuration and choose an\n" > "explicit 'smt=<bool>' setting. See XSA-439.\n"); > } > +#endif /* CONFIG_AMD */ And then no, I don't think we want to use #ifdef-ary here. IS_ENABLED() inside the functions (where the vendor checks are) is not only making sure the compiler will still parse all the code even when either vendor's support was turned off, but will also help review (by having in context what the actual vendor checks are in each function). Jan
On 16/08/2024 12:14 pm, Sergiy Kibrik wrote: > Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when > corresponding CPU support is disabled by configuration less dead code will end > up in the build. > > This includes re-ordering of calls to ibpb_calculations() & div_calculations(), > but since they don't access common variables or feature bits it should be > safe to do. > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > CC: Jan Beulich <jbeulich@suse.com> Sorry, but no. This logic is security critical, highly fragile, gets chopped/changed multiple times a year (as researchers keep on finding new things), and all major work is done to it under embargo. Just look at the history of the file. The ifdefary around the tsx_init() call is bad enough and I intend to revert it and do that differently. I would have objected if I'd got to the patch in time. The only relevant cost in this file is whether I (and the other security team members) can edit it correctly or not in staging and all prior in-support branches. You really don't want to know how many times there's been a bug in backports... Saving 451 lines from certification is not cheaper than the problems/risks you're introducing with this change. ~Andrew
On 29.08.2024 21:25, Andrew Cooper wrote: > On 16/08/2024 12:14 pm, Sergiy Kibrik wrote: >> Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when >> corresponding CPU support is disabled by configuration less dead code will end >> up in the build. >> >> This includes re-ordering of calls to ibpb_calculations() & div_calculations(), >> but since they don't access common variables or feature bits it should be >> safe to do. >> >> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> > > Sorry, but no. > > This logic is security critical, highly fragile, gets chopped/changed > multiple times a year (as researchers keep on finding new things), and > all major work is done to it under embargo. > > Just look at the history of the file. > > The ifdefary around the tsx_init() call is bad enough and I intend to > revert it and do that differently. I would have objected if I'd got to > the patch in time. > > > The only relevant cost in this file is whether I (and the other security > team members) can edit it correctly or not in staging and all prior > in-support branches. You really don't want to know how many times > there's been a bug in backports... > > Saving 451 lines from certification is not cheaper than the > problems/risks you're introducing with this change. Did you see my earlier reply? I don't think the issue is with hiding source lines. We want to have the compiler DCE stuff wherever possible, hence why I did respond asking to switch to IS_ENABLED(). That imo fits pretty well with the vendor checks we have there already anyway. Jan
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 75a4177a75..ba6c3e80d2 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -1012,6 +1012,7 @@ static bool __init should_use_eager_fpu(void) } } +#ifdef CONFIG_AMD /* * https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf */ @@ -1110,6 +1111,7 @@ static void __init div_calculations(bool hw_smt_enabled) "enabled. Please assess your configuration and choose an\n" "explicit 'smt=<bool>' setting. See XSA-439.\n"); } +#endif /* CONFIG_AMD */ static void __init ibpb_calculations(void) { @@ -1319,6 +1321,7 @@ static __init void l1tf_calculations(void) : (3UL << (paddr_bits - 2)))); } +#ifdef CONFIG_INTEL /* Calculate whether this CPU is vulnerable to MDS. */ static __init void mds_calculations(void) { @@ -1730,6 +1733,8 @@ static void __init bhi_calculations(void) } } +#endif /* CONFIG_INTEL */ + void spec_ctrl_init_domain(struct domain *d) { bool pv = is_pv_domain(d); @@ -2025,11 +2030,13 @@ void __init init_speculation_mitigations(void) default_scf |= SCF_ist_rsb; } +#ifdef CONFIG_AMD srso_calculations(hw_smt_enabled); - ibpb_calculations(); - div_calculations(hw_smt_enabled); +#endif + + ibpb_calculations(); /* Check whether Eager FPU should be enabled by default. */ if ( opt_eager_fpu == -1 ) @@ -2136,9 +2143,10 @@ void __init init_speculation_mitigations(void) * - March 2023, for RFDS. Enumerate RFDS_CLEAR to mean that VERW now * scrubs non-architectural entries from certain register files. */ +#ifdef CONFIG_INTEL mds_calculations(); rfds_calculations(); - +#endif /* * Parts which enumerate FB_CLEAR are those with now-updated microcode * which weren't susceptible to the original MFBDS (and therefore didn't @@ -2255,7 +2263,6 @@ void __init init_speculation_mitigations(void) opt_tsx = 0; tsx_init(); } -#endif /* * On some SRBDS-affected hardware, it may be safe to relax srb-lock by @@ -2286,6 +2293,8 @@ void __init init_speculation_mitigations(void) bhi_calculations(); +#endif /* CONFIG_INTEL */ + print_details(thunk); /*
Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when corresponding CPU support is disabled by configuration less dead code will end up in the build. This includes re-ordering of calls to ibpb_calculations() & div_calculations(), but since they don't access common variables or feature bits it should be safe to do. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/spec_ctrl.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)