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

[Jan Beulich <jbeulich@xxxxxxxx>]

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