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

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

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

Because:
#1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or 
not possible (PVH dom0).
#2 the changes to the Intel code are minimal.
#3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not 
perfect but far from fatal or breaking.

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.

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

>>>> @@ -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
> 
> 
Noted.

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