Re: [PATCH v4] x86/mwait-idle: re-order state entry/exit code a little

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.02.2022 10:08, Roger Pau Monné wrote:
> On Fri, Feb 18, 2022 at 09:35:10AM +0100, Jan Beulich wrote:
>> The initial observation is that unlike the original ACPI idle driver we
>> have a 2nd cpu_is_haltable() in here. By making the actual state entry
>> conditional, the emitted trace records as well as the subsequent stats
>> update are at least misleading in case the state wasn't actually entered.
>> Hence they would want moving inside the conditional. At which point the
>> cpuidle_get_tick() invocations could (and hence should) move as well.
>> cstate_restore_tsc() also isn't needed if we didn't actually enter the
>> state.
>>
>> This leaves only the errata_c6_workaround() and lapic_timer_off()
>> invocations outside the conditional. As a result it looks easier to
>> drop the conditional (and come back in sync with the other driver again)
>> than to move almost everything into the conditional.
>>
>> While there also move the TRACE_6D() out of the IRQ-disabled region.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -847,26 +847,25 @@ static void mwait_idle(void)
>>  
>>      update_last_cx_stat(power, cx, before);
>>  
>> -    if (cpu_is_haltable(cpu)) {
>> -            if (cx->irq_enable_early)
>> -                    local_irq_enable();
>> +    if (cx->irq_enable_early)
>> +            local_irq_enable();
> 
> Now that I look at this again, we need to be careful with the enabling
> interrupts and the interaction with errata_c6_workaround.  Enabling
> interrupts here could change the result of the check for pending EOIs,
> and thus enter mwait with a condition that could trigger the erratas.
> Hopefully CPUIDLE_FLAG_IRQ_ENABLE is only set for C1.
> 
> It might be prudent to only allow setting CPUIDLE_FLAG_IRQ_ENABLE for
> states <= 2.

Well, the justification for enabling was the low exit time. I don't
expect states > 2 to satisfy this criteria, so I think we're okay
without further precautions added right away.

Jan