Re: [Xen-devel] [PATCH 1/3] mwait-idle: add support for using halt

["Woods, Brian" <Brian.Woods@xxxxxxx>]

On 2/27/19 7:47 AM, Wei Liu wrote:
> On Mon, Feb 25, 2019 at 08:23:58PM +0000, Woods, Brian wrote:
>> Some AMD processors can use a mixture of mwait and halt for accessing
>> various c-states.  In preparation for adding support for AMD processors,
>> update the mwait-idle driver to optionally use halt.
>>
>> Signed-off-by: Brian Woods <brian.woods@xxxxxxx>
>> ---
>>   xen/arch/x86/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
>> index f89c52f256..a063e39d60 100644
>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -103,6 +103,11 @@ static const struct cpuidle_state {
>>   
>>   #define CPUIDLE_FLAG_DISABLED              0x1
>>   /*
>> + * On certain AMD families that support mwait, only c1 can be reached by
>> + * mwait and to reach c2, halt has to be used.
>> + */
>> +#define CPUIDLE_FLAG_USE_HALT               0x2
>> +/*
>>    * Set this flag for states where the HW flushes the TLB for us
>>    * and so we don't need cross-calls to keep it consistent.
>>    * If this flag is set, SW flushes the TLB, so even if the
>> @@ -783,8 +788,23 @@ static void mwait_idle(void)
>>   
>>      update_last_cx_stat(power, cx, before);
>>   
>> -    if (cpu_is_haltable(cpu))
>> -            mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
>> +    if (cpu_is_haltable(cpu)) {
>> +            struct cpu_info *info;
>> +            switch (cx->entry_method) {
>> +            case ACPI_CSTATE_EM_FFH:
>> +                    mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
>> +                    break;
>> +            case ACPI_CSTATE_EM_HALT:
> 
>> +                    info = get_cpu_info();
>> +                    spec_ctrl_enter_idle(info);
>> +                    safe_halt();
>> +                    spec_ctrl_exit_idle(info);
> 
> May I suggest you make this snippet a function? The same code snippet
> appears a few lines above.
> 
> Wei.
> 
It's used in various other places as well (cpu_idle.c, x86/domain.c), 
would a function like:

void safe_halt_with_spec(cpu_info *info)
{
     if (!info)
         info = get_cpu_info();

     spec_ctrl_enter_idle(info);
     safe_halt();
     spec_ctrl_exit_idle(info);
}

work since that way it could be used in other places where info is 
already defined?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel