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

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

On 3/13/19 4:35 AM, Jan Beulich wrote:
>>>> On 25.02.19 at 21:23, <Brian.Woods@xxxxxxx> wrote:
>> --- 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
> 
> Could you point us at where in the manuals this behavior is described?
> While PM Vol 2 has a chapter talking about P-states, I can't seem to
> find any mention of C-states there.

IIRC it's in the NDA PPR and internally it's in some other documents. 
We don't have support to use mwait while in CC6 due to caches being 
turned off etc.  If we did have mwait suport for CC6, we'd use that here 
(basically mirroring Intel).  Sadly I don't think we have any public 
information directly detailing this information.  If you'd like, I can 
look further into it.

>> @@ -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) {
> 
> Blank line between declaration(s) and statement(s) please. And
> it would seem better to move the declaration right here (inside
> the switch()) anyway. Then again ...
> 
>> +            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);
> 
> ... wouldn't it be better to avoid the redundancy with default_idle(),
> by introducing a new helper function, e.g. spec_ctrl_safe_halt()?
> 
See my email with Wei about this.


>> +                    local_irq_disable();
>> +                    break;
>> +            default:
>> +                    printk(XENLOG_ERR PREFIX "unknown entry method %d\n", 
>> cx->entry_method);
>> +            }
> 
> Overly long line and missing break statement.
> 
>> @@ -1184,8 +1204,9 @@ static int mwait_idle_cpu_init(struct notifier_block 
>> *nfb,
>>      for (cstate = 0; cpuidle_state_table[cstate].target_residency; 
>> ++cstate) {
>>              unsigned int num_substates, hint, state;
>>              struct acpi_processor_cx *cx;
>> +            const unsigned int flags = cpuidle_state_table[cstate].flags;
> 
> May I suggest to name the variable slightly differently, e.g. cflags,
> to avoid any risk of it being mistaken for what we commonly use
> with e.g. spin_lock_irqsave()?
> 
>> @@ -1221,7 +1242,12 @@ static int mwait_idle_cpu_init(struct notifier_block 
>> *nfb,
>>              cx = dev->states + dev->count;
>>              cx->type = state;
>>              cx->address = hint;
>> -            cx->entry_method = ACPI_CSTATE_EM_FFH;
>> +
>> +            if (flags & CPUIDLE_FLAG_USE_HALT)
>> +                    cx->entry_method = ACPI_CSTATE_EM_HALT;
>> +             else
>> +                    cx->entry_method = ACPI_CSTATE_EM_FFH;
> 
> I'd prefer if you used a conditional expression here. One of the goals for
> any changes to this file should be to limit the delta to its Linux original, 
> in
> order to increase the chances of patches coming from there to apply
> reasonably cleanly here.
> 
> Doing so would also save me from complaining about the stray blank
> ahead of "else".
> 
> Jan
> 

By conditional statement you mean ternary?  If so, that'll be easy enough.

Also, noted for things I didn't directly to.

Brian

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