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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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