[PATCH v2 0/3] x86/spec: improve command line parsing

[PATCH v2 0/3] x86/spec: improve command line parsing

[Roger Pau Monne]

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

[PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param()

[Roger Pau Monne]

'*' 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

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

[Roger Pau Monne]

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

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

[Roger Pau Monne]

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

Re: [PATCH v2 1/3] xen/cmdline: fix printf format specifier in no_config_param()

[Jan Beulich]

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>

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

[Jan Beulich]

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

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

[Jan Beulich]

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. */

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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