Re: [PATCH 6/6] x86/idle: Fix buggy "x86/mwait-idle: enable interrupts before C1 on Xeons"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2025 14:10, Andrew Cooper wrote:
> On 03/07/2025 10:43 am, Jan Beulich wrote:
>> On 02.07.2025 16:41, Andrew Cooper wrote:
>>> @@ -461,12 +468,19 @@ void mwait_idle_with_hints(unsigned int eax, unsigned 
>>> int ecx)
>>>  
>>>      monitor(this_softirq_pending, 0, 0);
>>>  
>>> +    ASSERT(!local_irq_is_enabled());
>>> +
>>>      if ( !*this_softirq_pending )
>>>      {
>>>          struct cpu_info *info = get_cpu_info();
>>>  
>>>          spec_ctrl_enter_idle(info);
>>> -        mwait(eax, ecx);
>>> +
>>> +        if ( ecx & MWAIT_ECX_INTERRUPT_BREAK )
>>> +            mwait(eax, ecx);
>>> +        else
>>> +            sti_mwait_cli(eax, ecx);
>> Actually, I'm curious: It seems quite likely that you did consider an
>> alternative resulting in assembly code like this:
>>
>>      test    $MWAIT_ECX_INTERRUPT_BREAK, %cl
>>      jz      0f
>>      sti
>> 0:
>>      monitor
>>      cli
>>
>> CLI being a relatively cheap operation aiui, is there anything wrong or
>> undesirable with this (smaller overall) alternative?
> 
> Other than it needing to be mwait?

Oops.

>  The overheads aren't interesting;
> they're nothing compared to going idle.
> 
> What does matter is that such a transformation cannot exist in mwait()
> itself, as that breaks acpi_dead_idle(), and if we turn this mwait()
> into inline asm, there's only a single caller of mwait() left.

I was rather think of it living in something derived from sti_mwait_cli(),
which could then be called unconditionally from mwait_idle_with_hints().

Jan