Re: [PATCH v2 2/3] x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.