* [PATCH v2 0/3] x86/spec: improve command line parsing
@ 2024-02-23 12:06 Roger Pau Monne
2024-02-23 12:06 ` [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() Roger Pau Monne
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Roger Pau Monne @ 2024-02-23 12:06 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
Hello,
A couple of improvements for the spec-ctrl command line parsing, and one
bugfix for no_config_param().
Thanks, Roger.
Roger Pau Monne (3):
xen/cmdline: fix printf format specifier in no_config_param()
x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled
x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled
xen/arch/x86/spec_ctrl.c | 15 ++++++++++++++-
xen/include/xen/param.h | 2 +-
2 files changed, 15 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() 2024-02-23 12:06 [PATCH v2 0/3] x86/spec: improve command line parsing Roger Pau Monne @ 2024-02-23 12:06 ` Roger Pau Monne 2024-02-26 8:29 ` Jan Beulich 2024-02-23 12:06 ` [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled Roger Pau Monne 2024-02-23 12:06 ` [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK " Roger Pau Monne 2 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monne @ 2024-02-23 12:06 UTC (permalink / raw) To: xen-devel Cc: Roger Pau Monne, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu '*' sets the width field, which is the minimum number of characters to output, but what we want in no_config_param() is the precision instead, which is '.*' as it imposes a maximum limit on the output. Fixes: 68d757df8dd2 ('x86/pv: Options to disable and/or compile out 32bit PV support') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/include/xen/param.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h index 9170455cde5c..6442a92aff8e 100644 --- a/xen/include/xen/param.h +++ b/xen/include/xen/param.h @@ -191,7 +191,7 @@ static inline void no_config_param(const char *cfg, const char *param, { int len = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s); - printk(XENLOG_INFO "CONFIG_%s disabled - ignoring '%s=%*s' setting\n", + printk(XENLOG_INFO "CONFIG_%s disabled - ignoring '%s=%.*s' setting\n", cfg, param, len, s); } -- 2.43.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() 2024-02-23 12:06 ` [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() Roger Pau Monne @ 2024-02-26 8:29 ` Jan Beulich 0 siblings, 0 replies; 12+ messages in thread From: Jan Beulich @ 2024-02-26 8:29 UTC (permalink / raw) To: Roger Pau Monne Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel On 23.02.2024 13:06, Roger Pau Monne wrote: > '*' sets the width field, which is the minimum number of characters to output, > but what we want in no_config_param() is the precision instead, which is '.*' > as it imposes a maximum limit on the output. > > Fixes: 68d757df8dd2 ('x86/pv: Options to disable and/or compile out 32bit PV support') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-23 12:06 [PATCH v2 0/3] x86/spec: improve command line parsing Roger Pau Monne 2024-02-23 12:06 ` [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() Roger Pau Monne @ 2024-02-23 12:06 ` Roger Pau Monne 2024-02-26 8:33 ` Jan Beulich 2024-02-26 8:42 ` Jan Beulich 2024-02-23 12:06 ` [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK " Roger Pau Monne 2 siblings, 2 replies; 12+ messages in thread From: Roger Pau Monne @ 2024-02-23 12:06 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu The current logic to handle the BRANCH_HARDEN option will report it as enabled even when build-time disabled. Fix this by only allowing the option to be set when support for it is built into Xen. Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Use no_config_param(). --- xen/arch/x86/spec_ctrl.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 421fe3f640df..4ce8a7a0b8ef 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; int8_t __read_mostly opt_eager_fpu = -1; int8_t __read_mostly opt_l1d_flush = -1; -static bool __initdata opt_branch_harden = true; +static bool __initdata opt_branch_harden = + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); bool __initdata bsp_delay_spec_ctrl; uint8_t __read_mostly default_xen_spec_ctrl; @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) opt_l1d_flush = val; else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) + { +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH opt_branch_harden = val; +#else + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); + rc = -EINVAL; +#endif + } else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) opt_srb_lock = val; else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) -- 2.43.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-23 12:06 ` [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled Roger Pau Monne @ 2024-02-26 8:33 ` Jan Beulich 2024-02-26 8:42 ` Jan Beulich 1 sibling, 0 replies; 12+ messages in thread From: Jan Beulich @ 2024-02-26 8:33 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel On 23.02.2024 13:06, Roger Pau Monne wrote: > The current logic to handle the BRANCH_HARDEN option will report it as enabled > even when build-time disabled. Fix this by only allowing the option to be set > when support for it is built into Xen. > > Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Use no_config_param(). > --- > xen/arch/x86/spec_ctrl.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > index 421fe3f640df..4ce8a7a0b8ef 100644 > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > int8_t __read_mostly opt_eager_fpu = -1; > int8_t __read_mostly opt_l1d_flush = -1; > -static bool __initdata opt_branch_harden = true; > +static bool __initdata opt_branch_harden = > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > bool __initdata bsp_delay_spec_ctrl; > uint8_t __read_mostly default_xen_spec_ctrl; > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > opt_l1d_flush = val; > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > + { > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > opt_branch_harden = val; > +#else > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > + rc = -EINVAL; > +#endif > + } If you use #ifdef (rather than IS_ENABLED()) here, the variable probably shouldn't exist at all when the Kconfig option is off (albeit yet, another #ifdef would then be needed higher up in the function). Question is - why don't you use IS_ENABLED() here as well? Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-23 12:06 ` [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled Roger Pau Monne 2024-02-26 8:33 ` Jan Beulich @ 2024-02-26 8:42 ` Jan Beulich 2024-02-26 9:09 ` Roger Pau Monné 1 sibling, 1 reply; 12+ messages in thread From: Jan Beulich @ 2024-02-26 8:42 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel On 23.02.2024 13:06, Roger Pau Monne wrote: > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > int8_t __read_mostly opt_eager_fpu = -1; > int8_t __read_mostly opt_l1d_flush = -1; > -static bool __initdata opt_branch_harden = true; > +static bool __initdata opt_branch_harden = > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > bool __initdata bsp_delay_spec_ctrl; > uint8_t __read_mostly default_xen_spec_ctrl; > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > opt_l1d_flush = val; > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > + { > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > opt_branch_harden = val; > +#else > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > + rc = -EINVAL; > +#endif > + } > else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > opt_srb_lock = val; > else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) While looking at patch 3 I noticed another inconsistency: Wouldn't "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just like for thunks both CONFIG_* state and actual runtime choice are logged? Or alternatively, should logging of thunk runtime choice be suppressed when the Kconfig setting is off? Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-26 8:42 ` Jan Beulich @ 2024-02-26 9:09 ` Roger Pau Monné 2024-02-26 9:33 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2024-02-26 9:09 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: > On 23.02.2024 13:06, Roger Pau Monne wrote: > > --- a/xen/arch/x86/spec_ctrl.c > > +++ b/xen/arch/x86/spec_ctrl.c > > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > > int8_t __read_mostly opt_eager_fpu = -1; > > int8_t __read_mostly opt_l1d_flush = -1; > > -static bool __initdata opt_branch_harden = true; > > +static bool __initdata opt_branch_harden = > > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > > > bool __initdata bsp_delay_spec_ctrl; > > uint8_t __read_mostly default_xen_spec_ctrl; > > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > > opt_l1d_flush = val; > > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > + { > > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > > opt_branch_harden = val; > > +#else > > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > > + rc = -EINVAL; > > +#endif > > + } > > else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > > opt_srb_lock = val; > > else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) > > While looking at patch 3 I noticed another inconsistency: Wouldn't > "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just > like for thunks both CONFIG_* state and actual runtime choice are > logged? Yes, I guess we would also need to expand "Compiled-in support:" to include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > Or alternatively, should logging of thunk runtime choice be > suppressed when the Kconfig setting is off? Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has been built time disabled. Thanks, Roger. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-26 9:09 ` Roger Pau Monné @ 2024-02-26 9:33 ` Jan Beulich 2024-02-26 9:57 ` Roger Pau Monné 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2024-02-26 9:33 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Wei Liu, xen-devel On 26.02.2024 10:09, Roger Pau Monné wrote: > On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: >> On 23.02.2024 13:06, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/spec_ctrl.c >>> +++ b/xen/arch/x86/spec_ctrl.c >>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; >>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; >>> int8_t __read_mostly opt_eager_fpu = -1; >>> int8_t __read_mostly opt_l1d_flush = -1; >>> -static bool __initdata opt_branch_harden = true; >>> +static bool __initdata opt_branch_harden = >>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); >>> >>> bool __initdata bsp_delay_spec_ctrl; >>> uint8_t __read_mostly default_xen_spec_ctrl; >>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>> opt_l1d_flush = val; >>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>> + { >>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH >>> opt_branch_harden = val; >>> +#else >>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); >>> + rc = -EINVAL; >>> +#endif >>> + } >>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) >>> opt_srb_lock = val; >>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) >> >> While looking at patch 3 I noticed another inconsistency: Wouldn't >> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just >> like for thunks both CONFIG_* state and actual runtime choice are >> logged? > > Yes, I guess we would also need to expand "Compiled-in support:" to > include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > >> Or alternatively, should logging of thunk runtime choice be >> suppressed when the Kconfig setting is off? > > Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has > been built time disabled. Good enough - yes. But redundant with the other log line. And since all of this output is getting longer and longer anyway, omitting whatever can sensibly be omitted might be at least worth considering. Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-26 9:33 ` Jan Beulich @ 2024-02-26 9:57 ` Roger Pau Monné 2024-02-26 10:16 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2024-02-26 9:57 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel On Mon, Feb 26, 2024 at 10:33:14AM +0100, Jan Beulich wrote: > On 26.02.2024 10:09, Roger Pau Monné wrote: > > On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: > >> On 23.02.2024 13:06, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/spec_ctrl.c > >>> +++ b/xen/arch/x86/spec_ctrl.c > >>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > >>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > >>> int8_t __read_mostly opt_eager_fpu = -1; > >>> int8_t __read_mostly opt_l1d_flush = -1; > >>> -static bool __initdata opt_branch_harden = true; > >>> +static bool __initdata opt_branch_harden = > >>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > >>> > >>> bool __initdata bsp_delay_spec_ctrl; > >>> uint8_t __read_mostly default_xen_spec_ctrl; > >>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > >>> opt_l1d_flush = val; > >>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > >>> + { > >>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > >>> opt_branch_harden = val; > >>> +#else > >>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > >>> + rc = -EINVAL; > >>> +#endif > >>> + } > >>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > >>> opt_srb_lock = val; > >>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) > >> > >> While looking at patch 3 I noticed another inconsistency: Wouldn't > >> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just > >> like for thunks both CONFIG_* state and actual runtime choice are > >> logged? > > > > Yes, I guess we would also need to expand "Compiled-in support:" to > > include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > > > >> Or alternatively, should logging of thunk runtime choice be > >> suppressed when the Kconfig setting is off? > > > > Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has > > been built time disabled. > > Good enough - yes. But redundant with the other log line. And since all > of this output is getting longer and longer anyway, omitting whatever > can sensibly be omitted might be at least worth considering. I can add yet another patch to remove that when not built-in, this cleanup is turning into a never-ending exercise I'm afraid. Thanks, Roger. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled 2024-02-26 9:57 ` Roger Pau Monné @ 2024-02-26 10:16 ` Jan Beulich 0 siblings, 0 replies; 12+ messages in thread From: Jan Beulich @ 2024-02-26 10:16 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Wei Liu, xen-devel On 26.02.2024 10:57, Roger Pau Monné wrote: > On Mon, Feb 26, 2024 at 10:33:14AM +0100, Jan Beulich wrote: >> On 26.02.2024 10:09, Roger Pau Monné wrote: >>> On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: >>>> On 23.02.2024 13:06, Roger Pau Monne wrote: >>>>> --- a/xen/arch/x86/spec_ctrl.c >>>>> +++ b/xen/arch/x86/spec_ctrl.c >>>>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; >>>>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; >>>>> int8_t __read_mostly opt_eager_fpu = -1; >>>>> int8_t __read_mostly opt_l1d_flush = -1; >>>>> -static bool __initdata opt_branch_harden = true; >>>>> +static bool __initdata opt_branch_harden = >>>>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); >>>>> >>>>> bool __initdata bsp_delay_spec_ctrl; >>>>> uint8_t __read_mostly default_xen_spec_ctrl; >>>>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>>>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>>>> opt_l1d_flush = val; >>>>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>>>> + { >>>>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH >>>>> opt_branch_harden = val; >>>>> +#else >>>>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); >>>>> + rc = -EINVAL; >>>>> +#endif >>>>> + } >>>>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) >>>>> opt_srb_lock = val; >>>>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) >>>> >>>> While looking at patch 3 I noticed another inconsistency: Wouldn't >>>> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just >>>> like for thunks both CONFIG_* state and actual runtime choice are >>>> logged? >>> >>> Yes, I guess we would also need to expand "Compiled-in support:" to >>> include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. >>> >>>> Or alternatively, should logging of thunk runtime choice be >>>> suppressed when the Kconfig setting is off? >>> >>> Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has >>> been built time disabled. >> >> Good enough - yes. But redundant with the other log line. And since all >> of this output is getting longer and longer anyway, omitting whatever >> can sensibly be omitted might be at least worth considering. > > I can add yet another patch to remove that when not built-in, this > cleanup is turning into a never-ending exercise I'm afraid. Just to be clear: I'm not meaning to insist that you also clean up the thunk part. But I still wanted to mention such as I noticed it. Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled 2024-02-23 12:06 [PATCH v2 0/3] x86/spec: improve command line parsing Roger Pau Monne 2024-02-23 12:06 ` [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() Roger Pau Monne 2024-02-23 12:06 ` [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled Roger Pau Monne @ 2024-02-23 12:06 ` Roger Pau Monne 2024-02-26 8:40 ` Jan Beulich 2 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monne @ 2024-02-23 12:06 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu Attempt to provide a more helpful error message when the user attempts to set spec-ctrl=bti-thunk option but the support is build-time disabled. Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/spec_ctrl.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 4ce8a7a0b8ef..f3432f1a6c80 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -239,6 +239,7 @@ static int __init cf_check parse_spec_ctrl(const char *s) /* Xen's speculative sidechannel mitigation settings. */ else if ( !strncmp(s, "bti-thunk=", 10) ) { +#ifdef CONFIG_INDIRECT_THUNK s += 10; if ( !cmdline_strcmp(s, "retpoline") ) @@ -249,6 +250,10 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_thunk = THUNK_JMP; else rc = -EINVAL; +#else + no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss); + rc = -EINVAL; +#endif } /* Bits in MSR_SPEC_CTRL. */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled 2024-02-23 12:06 ` [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK " Roger Pau Monne @ 2024-02-26 8:40 ` Jan Beulich 0 siblings, 0 replies; 12+ messages in thread From: Jan Beulich @ 2024-02-26 8:40 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel On 23.02.2024 13:06, Roger Pau Monne wrote: > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -239,6 +239,7 @@ static int __init cf_check parse_spec_ctrl(const char *s) > /* Xen's speculative sidechannel mitigation settings. */ > else if ( !strncmp(s, "bti-thunk=", 10) ) > { > +#ifdef CONFIG_INDIRECT_THUNK > s += 10; > > if ( !cmdline_strcmp(s, "retpoline") ) if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) { no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss); rc = -EINVAL; } else if ( !cmdline_strcmp(s, "retpoline") ) ? Also, while touching INDIRECT_THUNK, could I talk you into also adjusting documentation accordingly? It presently mentions INDIRECT_THUNK, when really it means CONFIG_INDIRECT_THUNK. Jan > @@ -249,6 +250,10 @@ static int __init cf_check parse_spec_ctrl(const char *s) > opt_thunk = THUNK_JMP; > else > rc = -EINVAL; > +#else > + no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss); > + rc = -EINVAL; > +#endif > } > > /* Bits in MSR_SPEC_CTRL. */ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-02-26 10:16 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-23 12:06 [PATCH v2 0/3] x86/spec: improve command line parsing Roger Pau Monne 2024-02-23 12:06 ` [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param() Roger Pau Monne 2024-02-26 8:29 ` Jan Beulich 2024-02-23 12:06 ` [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled Roger Pau Monne 2024-02-26 8:33 ` Jan Beulich 2024-02-26 8:42 ` Jan Beulich 2024-02-26 9:09 ` Roger Pau Monné 2024-02-26 9:33 ` Jan Beulich 2024-02-26 9:57 ` Roger Pau Monné 2024-02-26 10:16 ` Jan Beulich 2024-02-23 12:06 ` [PATCH v2 3/3] x86/spec: fix INDIRECT_THUNK " Roger Pau Monne 2024-02-26 8:40 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.