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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.19 at 20:00, <Brian.Woods@xxxxxxx> wrote:
> 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.

Ah yes, I found it. But the text suggests to use SystemIO, not
HLT for entering C2 (CC6). An important difference looks to be
the state of EFLAGS.IF as to whether the core wakes up again.
The SystemIO approach would better match the FFixedHW one,
as we require and use MWAIT_ECX_INTERRUPT_BREAK.

Furthermore I'm then once again wondering what the gain is
over using the ACPI driver: The suggested _CST looks to exactly
match the data you enter into the table in the later patch. IOW
my fundamental concern didn't go away yet: As per the name
of the driver, it shouldn't really need to support HLT (or anything
other than MWAIT) as an entry method. Hence I think that at
the very least you need to extend the description of the change
quite a bit to explain why the ACPI driver is not suitable.

Depending on how this comes out, it may then still be a matter
of discussing whether, rather than fiddling with mwait-idle, it
wouldn't be better to have an AMD-specific driver instead. Are
there any thoughts in similar directions for Linux?

>>> +           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.

There you've basically settled on making a helper function, to
be used in pre-existing places as well as here.

I've also just noticed that there's another safe_halt() invocation
a few lines up from here, as a fallback. It doesn't come with any
of the statistics though, so would probably be unsuitable to
funnel into.

>>> @@ -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".
> 
> By conditional statement you mean ternary?  If so, that'll be easy enough.

Yes.

Jan



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