Most of the patch (handling of CPUIDLE_FLAG_IBRS) is fine, but the
adjustements to mwait_idle() are not.

spec_ctrl_{enter,exit}_idle() do more than just alter MSR_SPEC_CTRL.IBRS.  The
VERW and RSB stuff are **unsafe** to omit.

The only reason this doesn't need an XSA is because no changes were made to
the lower level mwait_idle_with_hints(), and thus it remained properly
protected.

I.e. This change only served to double the expensive operations in the case it
was trying to optimise.

I have an idea of how to plumb this more nicely, but it requires larger
changes to legacy IBRS handling to not make spec_ctrl_enter_idle() vulnerable
in other ways.  In the short term, simply take out the perf hit.

Fixes: 08acdf9a2615 ("x86/mwait-idle: disable IBRS during long idle")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/mwait-idle.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 9c16cc166a14..5c16f5ad3a82 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -875,7 +875,6 @@ static const struct cpuidle_state snr_cstates[] = {
 static void cf_check mwait_idle(void)
 {
 	unsigned int cpu = smp_processor_id();
-	struct cpu_info *info = get_cpu_info();
 	struct acpi_processor_power *power = processor_powers[cpu];
 	struct acpi_processor_cx *cx = NULL;
 	unsigned int next_state;
@@ -902,6 +901,8 @@ static void cf_check mwait_idle(void)
 			pm_idle_save();
 		else
 		{
+			struct cpu_info *info = get_cpu_info();
+
 			spec_ctrl_enter_idle(info);
 			safe_halt();
 			spec_ctrl_exit_idle(info);
@@ -928,11 +929,6 @@ static void cf_check mwait_idle(void)
 	if ((cx->type >= 3) && errata_c6_workaround())
 		cx = power->safe_state;
 
-	if (cx->ibrs_disable) {
-		ASSERT(!cx->irq_enable_early);
-		spec_ctrl_enter_idle(info);
-	}
-
 #if 0 /* XXX Can we/do we need to do something similar on Xen? */
 	/*
 	 * leave_mm() to avoid costly and often unnecessary wakeups
@@ -964,10 +960,6 @@ static void cf_check mwait_idle(void)
 
 	/* Now back in C0. */
 	update_idle_stats(power, cx, before, after);
-
-	if (cx->ibrs_disable)
-		spec_ctrl_exit_idle(info);
-
 	local_irq_enable();
 
 	TRACE_TIME(TRC_PM_IDLE_EXIT, cx->type, after,
-- 
2.39.5

On 24.06.2025 18:39, Andrew Cooper wrote:
> Most of the patch (handling of CPUIDLE_FLAG_IBRS) is fine, but the
> adjustements to mwait_idle() are not.
> 
> spec_ctrl_{enter,exit}_idle() do more than just alter MSR_SPEC_CTRL.IBRS.  The
> VERW and RSB stuff are **unsafe** to omit.
> 
> The only reason this doesn't need an XSA is because no changes were made to
> the lower level mwait_idle_with_hints(), and thus it remained properly
> protected.
> 
> I.e. This change only served to double the expensive operations in the case it
> was trying to optimise.
> 
> I have an idea of how to plumb this more nicely, but it requires larger
> changes to legacy IBRS handling to not make spec_ctrl_enter_idle() vulnerable
> in other ways.

What are the concerns here? As it looks skipping the MSR write would look
to require checking some (per-CPU) conditional. Conditional branches can't
really be of concern, or the "if (cx->ibrs_disable)" that you're now
removing again would have been of concern, too. Hence simply a new SCF_
flag would look to be sufficient, for mwait_idle() to convey the necessary
info to spec_ctrl_enter_idle()?

>  In the short term, simply take out the perf hit.
> 
> Fixes: 08acdf9a2615 ("x86/mwait-idle: disable IBRS during long idle")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

On 25/06/2025 10:58 am, Jan Beulich wrote:
> On 24.06.2025 18:39, Andrew Cooper wrote:
>> Most of the patch (handling of CPUIDLE_FLAG_IBRS) is fine, but the
>> adjustements to mwait_idle() are not.
>>
>> spec_ctrl_{enter,exit}_idle() do more than just alter MSR_SPEC_CTRL.IBRS.  The
>> VERW and RSB stuff are **unsafe** to omit.
>>
>> The only reason this doesn't need an XSA is because no changes were made to
>> the lower level mwait_idle_with_hints(), and thus it remained properly
>> protected.
>>
>> I.e. This change only served to double the expensive operations in the case it
>> was trying to optimise.
>>
>> I have an idea of how to plumb this more nicely, but it requires larger
>> changes to legacy IBRS handling to not make spec_ctrl_enter_idle() vulnerable
>> in other ways.
> What are the concerns here? As it looks skipping the MSR write would look
> to require checking some (per-CPU) conditional. Conditional branches can't
> really be of concern, or the "if (cx->ibrs_disable)" that you're now
> removing again would have been of concern, too.

The conditional branches are what set off alarm bells in the first place.

A conditional branch in enter should be ok; HLT and MWAIT should be
serialising enough.

A conditional branch in exit is not ok without extra safety measures.

I can expand on this in the commit message if you'd like.  I was trying
to not be overly critical...

>  Hence simply a new SCF_
> flag would look to be sufficient, for mwait_idle() to convey the necessary
> info to spec_ctrl_enter_idle()?

I've got a local patch going that route, but it needs more than just an
SCF flag.  This is the "requires larger changes".

>
>>  In the short term, simply take out the perf hit.
>>
>> Fixes: 08acdf9a2615 ("x86/mwait-idle: disable IBRS during long idle")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Thanks.

~Andrew

On 25.06.2025 15:01, Andrew Cooper wrote:
> On 25/06/2025 10:58 am, Jan Beulich wrote:
>> On 24.06.2025 18:39, Andrew Cooper wrote:
>>> Most of the patch (handling of CPUIDLE_FLAG_IBRS) is fine, but the
>>> adjustements to mwait_idle() are not.
>>>
>>> spec_ctrl_{enter,exit}_idle() do more than just alter MSR_SPEC_CTRL.IBRS.  The
>>> VERW and RSB stuff are **unsafe** to omit.
>>>
>>> The only reason this doesn't need an XSA is because no changes were made to
>>> the lower level mwait_idle_with_hints(), and thus it remained properly
>>> protected.
>>>
>>> I.e. This change only served to double the expensive operations in the case it
>>> was trying to optimise.
>>>
>>> I have an idea of how to plumb this more nicely, but it requires larger
>>> changes to legacy IBRS handling to not make spec_ctrl_enter_idle() vulnerable
>>> in other ways.
>> What are the concerns here? As it looks skipping the MSR write would look
>> to require checking some (per-CPU) conditional. Conditional branches can't
>> really be of concern, or the "if (cx->ibrs_disable)" that you're now
>> removing again would have been of concern, too.
> 
> The conditional branches are what set off alarm bells in the first place.
> 
> A conditional branch in enter should be ok; HLT and MWAIT should be
> serialising enough.
> 
> A conditional branch in exit is not ok without extra safety measures.
> 
> I can expand on this in the commit message if you'd like.  I was trying
> to not be overly critical...

For me, the answer here is sufficient, I guess. Hence I won't insist on you
amending the description. It may help others and/or some time into the
future, though.

Jan