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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.19 at 17:12, <Brian.Woods@xxxxxxx> wrote:
> On 3/15/19 3:37 AM, Jan Beulich wrote:
>>>>> 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?
> 
> I can make it use sysIO rather than HLT if there's a need or strong 
> desire for it.  I used HLT mainly because I thought it would be more 
> robust (like in the case of CC6 being disabled).

Well, you know whether to trust your documentation.

> Because:
> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or 
> not possible (PVH dom0).

Why unreliable? And PVH Dom0 is still WIP.

> #2 the changes to the Intel code are minimal.

But they go against the purpose of the file, which is to make use of
MWAIT.

> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not 
> perfect but far from fatal or breaking.

Yes, that's a minor aspect.

> In Linux, they have a working AML interrupter so they just read the ACPI 
> tables.  If Xen had a working AML interrupter, I'd suggest just reading 
> the ACPI tables as well.  As far as a completely different driver for 
> AMD, it would mostly just be the Intel drive with the small changes and 
> some code removed.  With the minimal changes needed, I don't see a 
> reason, but that's just me.

Well, I've put this up for discussion, and specifically raised the
question of what Linux is doing here.

>>>>> +         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.
> 
> It does use follow the pattern of:
>       spec_ctrl_enter_idle(info);
>       safe_halt();
>       spec_ctrl_exit_idle(info);
> though.  I'm pretty sure out would work with what I suggested or am I 
> missing something?

I'm afraid I'm not understanding the question in this context. Perhaps
I'm largely confused by the use of "out" in the middle of the sentence.
Is this referring to the OUT instruction in context of the SysIO aspect
discussed further up? If so - I don't understand what you're trying to
get at in the context here. If not, I'm lost altogether.

Jan


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