| Message ID | 20240226110748.80254-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/spec: improve command line parsing | expand |
On 26.02.2024 12:07, Roger Pau Monne wrote: > 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. > > While there also adjust the command line documentation to mention > CONFIG_INDIRECT_THUNK instead of INDIRECT_THUNK. > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one minor remark: > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -241,7 +241,12 @@ static int __init cf_check parse_spec_ctrl(const char *s) > { > s += 10; > > - if ( !cmdline_strcmp(s, "retpoline") ) > + if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) > + { > + no_config_param("INDIRECT_THUNK", "spec-ctrl=bti-thunk", s, ss); > + rc = -EINVAL; > + } > + else if ( !cmdline_strcmp(s, "retpoline") ) > opt_thunk = THUNK_RETPOLINE; > else if ( !cmdline_strcmp(s, "lfence") ) > opt_thunk = THUNK_LFENCE; How about if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) { no_config_param("INDIRECT_THUNK", "spec-ctrl", s - 10, ss); rc = -EINVAL; } else if ( !cmdline_strcmp(s, "retpoline") ) or (likely less liked by you and Andrew) "s += 10;" dropped and then if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) { no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss); rc = -EINVAL; } else if ( !cmdline_strcmp(s += 10, "retpoline") ) conserving a little on string literal space (sadly, despite the function being __init, string literals remain post-init due to living in .rodata)? Jan
On Mon, Feb 26, 2024 at 01:50:46PM +0100, Jan Beulich wrote: > On 26.02.2024 12:07, Roger Pau Monne wrote: > > 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. > > > > While there also adjust the command line documentation to mention > > CONFIG_INDIRECT_THUNK instead of INDIRECT_THUNK. > > > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one minor remark: > > > --- a/xen/arch/x86/spec_ctrl.c > > +++ b/xen/arch/x86/spec_ctrl.c > > @@ -241,7 +241,12 @@ static int __init cf_check parse_spec_ctrl(const char *s) > > { > > s += 10; > > > > - if ( !cmdline_strcmp(s, "retpoline") ) > > + if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) > > + { > > + no_config_param("INDIRECT_THUNK", "spec-ctrl=bti-thunk", s, ss); > > + rc = -EINVAL; > > + } > > + else if ( !cmdline_strcmp(s, "retpoline") ) > > opt_thunk = THUNK_RETPOLINE; > > else if ( !cmdline_strcmp(s, "lfence") ) > > opt_thunk = THUNK_LFENCE; > > How about > > if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) > { > no_config_param("INDIRECT_THUNK", "spec-ctrl", s - 10, ss); > rc = -EINVAL; > } > else if ( !cmdline_strcmp(s, "retpoline") ) > > or (likely less liked by you and Andrew) "s += 10;" dropped and then > > if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) > { > no_config_param("INDIRECT_THUNK", "spec-ctrl", s, ss); > rc = -EINVAL; > } > else if ( !cmdline_strcmp(s += 10, "retpoline") ) > > conserving a little on string literal space (sadly, despite the function > being __init, string literals remain post-init due to living in .rodata)? TBH I'm not overly worried about those 10 extra characters, but if I had to choose I prefer the first option (so `s - 10`). Thanks, Roger.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index be76be8d5365..02896598df6f 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2417,11 +2417,11 @@ guests to use. performance reasons dom0 is unprotected by default. If it is necessary to protect dom0 too, boot with `spec-ctrl=ibpb-entry`. -If Xen was compiled with INDIRECT_THUNK support, `bti-thunk=` can be used to -select which of the thunks gets patched into the `__x86_indirect_thunk_%reg` -locations. The default thunk is `retpoline` (generally preferred), with the -alternatives being `jmp` (a `jmp *%reg` gadget, minimal overhead), and -`lfence` (an `lfence; jmp *%reg` gadget). +If Xen was compiled with `CONFIG_INDIRECT_THUNK` support, `bti-thunk=` can be +used to select which of the thunks gets patched into the +`__x86_indirect_thunk_%reg` locations. The default thunk is `retpoline` +(generally preferred), with the alternatives being `jmp` (a `jmp *%reg` gadget, +minimal overhead), and `lfence` (an `lfence; jmp *%reg` gadget). On hardware supporting IBRS (Indirect Branch Restricted Speculation), the `ibrs=` option can be used to force or prevent Xen using the feature itself. diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 5fae80774519..ca82b9e41ccd 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -241,7 +241,12 @@ static int __init cf_check parse_spec_ctrl(const char *s) { s += 10; - if ( !cmdline_strcmp(s, "retpoline") ) + if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) ) + { + no_config_param("INDIRECT_THUNK", "spec-ctrl=bti-thunk", s, ss); + rc = -EINVAL; + } + else if ( !cmdline_strcmp(s, "retpoline") ) opt_thunk = THUNK_RETPOLINE; else if ( !cmdline_strcmp(s, "lfence") ) opt_thunk = THUNK_LFENCE;
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. While there also adjust the command line documentation to mention CONFIG_INDIRECT_THUNK instead of INDIRECT_THUNK. Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - Adjust documentation. - Use IS_ENABLED() instead of #ifdef. Changes since v1: - New in this version. --- docs/misc/xen-command-line.pandoc | 10 +++++----- xen/arch/x86/spec_ctrl.c | 7 ++++++- 2 files changed, 11 insertions(+), 6 deletions(-)