diff mbox series

[v3,3/4] x86/spec: fix INDIRECT_THUNK option to only be set when build-enabled

Message ID 20240226110748.80254-4-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/spec: improve command line parsing | expand

Commit Message

Roger Pau Monné Feb. 26, 2024, 11:07 a.m. UTC
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(-)

Comments

Jan Beulich Feb. 26, 2024, 12:50 p.m. UTC | #1
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
Roger Pau Monné Feb. 27, 2024, 11:18 a.m. UTC | #2
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 mbox series

Patch

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;