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

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

On 3/5/19 11:12 AM, Wei Liu wrote:
> On Wed, Feb 27, 2019 at 06:23:35PM +0000, Woods, Brian wrote:
>> 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?
> 
> Looks reasonable. But I will leave that to Andrew and Jan to decide what
> suits them best.
> 
> Wei.
> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxxxxxxxxx
>> https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping for Andy and Jan for this and the patches in general?

Brian
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel