linux-kernel - Re: [PATCH v6 25/26] intel_idle/amx: Clear the AMX state before entering idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0jn6pnwOj0tYO35kNj+WN5Htwc5gzA6GJdQDK+A3g4BLA@mail.gmail.com>
Date:   Wed, 30 Jun 2021 15:11:31 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     Borislav Petkov <bp@...e.de>, Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Dave Hansen <dave.hansen@...el.com>, jing2.liu@...el.com,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v6 25/26] intel_idle/amx: Clear the AMX state before
 entering idle

On Wed, Jun 30, 2021 at 8:08 AM Chang S. Bae <chang.seok.bae@...el.com> wrote:
>
> When AMX is enabled, initialize AMX state before going idle.
>
> This assures that the kernel will only request idle states with clean AMX
> state. In the case of the C6 idle state, this may allow the hardware to get
> to a deeper power saving condition.

At least the changelog needs to be improved, because the patch adds a
table of idle states for SPR, which is not mentioned here.

Also this is an optimization which is only done for SPR C6 in order to
increase the likelihood of entering this state in case the tile
registers are dirty when it is requested.

> [ Based on patch by Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>. ]
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Reviewed-by: Len Brown <len.brown@...el.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-pm@...r.kernel.org
> ---
> Changes from v5:
> * Moved the code to intel_idle. (Peter Zijlstra)
> * Fixed to deactivate fpregs. (Andy Lutomirski and Dave Hansen)
> * Updated the code comment. (Dave Hansen)
>
> Changes from v4:
> * Added as a new patch. (Thomas Gleixner)
> ---
>  arch/x86/include/asm/special_insns.h |  6 +++
>  drivers/idle/intel_idle.c            | 79 ++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index f3fbb84ff8a7..fada1bb82c7b 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -294,6 +294,12 @@ static inline int enqcmds(void __iomem *dst, const void *src)
>         return 0;
>  }
>
> +static inline void tile_release(void)
> +{
> +       /* Instruction opcode for TILERELEASE; supported in binutils >= 2.36. */
> +       asm volatile(".byte 0xc4, 0xe2, 0x78, 0x49, 0xc0");
> +}
> +
>  #endif /* __KERNEL__ */
>
>  #endif /* _ASM_X86_SPECIAL_INSNS_H */
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index ec1b9d306ba6..a77c6d7a10e5 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -54,6 +54,8 @@
>  #include <asm/intel-family.h>
>  #include <asm/mwait.h>
>  #include <asm/msr.h>
> +#include <asm/fpu/internal.h>
> +#include <asm/special_insns.h>
>
>  #define INTEL_IDLE_VERSION "0.5.1"
>
> @@ -155,6 +157,55 @@ static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev,
>         return 0;
>  }
>
> +/**
> + * idle_tile() - Initialize TILE registers in INIT-state
> + *
> + * Leaving state in the dirty TILE registers may prevent the processor from
> + * entering lower-power idle states. Use TILERELEASE to initialize the
> + * state. Destroying fpregs state is safe after the fpstate update.
> + *
> + * Returns:    None

This is redundant for a void function.

> + */
> +static inline void idle_tile(void)
> +{
> +       if (boot_cpu_has(X86_FEATURE_XGETBV1) && (xgetbv(1) & XFEATURE_MASK_XTILE)) {
> +               tile_release();
> +               fpregs_deactivate(&current->thread.fpu);
> +       }
> +}
> +
> +/**
> + * intel_idle_tile - Ask the processor to enter the given idle state.
> + * @dev: cpuidle device of the target CPU.
> + * @drv: cpuidle driver (assumed to point to intel_idle_driver).
> + * @index: Target idle state index.
> + *
> + * Ensure TILE registers in INIT-state.

This is not the only thing done by the function.

> + */
> +static __cpuidle int intel_idle_tile(struct cpuidle_device *dev,
> +                                    struct cpuidle_driver *drv, int index)
> +{
> +       idle_tile();
> +
> +       return intel_idle(dev, drv, index);
> +}
> +
> +/**
> + * intel_idle_s2idle_tile - Ask the processor to enter the given idle state.
> + * @dev: cpuidle device of the target CPU.
> + * @drv: cpuidle driver (assumed to point to intel_idle_driver).
> + * @index: Target idle state index.
> + *
> + * Ensure TILE registers in INIT-state.

Same here.

> + */
> +static __cpuidle int intel_idle_s2idle_tile(struct cpuidle_device *dev,
> +                                           struct cpuidle_driver *drv, int index)
> +{
> +       idle_tile();
> +
> +       return intel_idle_s2idle(dev, drv, index);
> +}
> +
>  /*
>   * States are indexed by the cstate number,
>   * which is also the index into the MWAIT hint array.
> @@ -752,6 +803,27 @@ static struct cpuidle_state icx_cstates[] __initdata = {
>                 .enter = NULL }
>  };
>
> +static struct cpuidle_state spr_cstates[] __initdata = {
> +       {
> +               .name = "C1",
> +               .desc = "MWAIT 0x00",
> +               .flags = MWAIT2flg(0x00),
> +               .exit_latency = 1,
> +               .target_residency = 1,
> +               .enter = &intel_idle,
> +               .enter_s2idle = intel_idle_s2idle, },
> +       {
> +               .name = "C6",
> +               .desc = "MWAIT 0x20",
> +               .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
> +               .exit_latency = 128,
> +               .target_residency = 384,
> +               .enter = &intel_idle_tile,
> +               .enter_s2idle = intel_idle_s2idle_tile, },
> +       {
> +               .enter = NULL }
> +};
> +
>  static struct cpuidle_state atom_cstates[] __initdata = {
>         {
>                 .name = "C1E",
> @@ -1095,6 +1167,12 @@ static const struct idle_cpu idle_cpu_icx __initconst = {
>         .use_acpi = true,
>  };
>
> +static const struct idle_cpu idle_cpu_spr __initconst = {
> +       .state_table = spr_cstates,
> +       .disable_promotion_to_c1e = true,
> +       .use_acpi = true,
> +};
> +
>  static const struct idle_cpu idle_cpu_avn __initconst = {
>         .state_table = avn_cstates,
>         .disable_promotion_to_c1e = true,
> @@ -1157,6 +1235,7 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
>         X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,           &idle_cpu_skx),
>         X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,           &idle_cpu_icx),
>         X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,           &idle_cpu_icx),
> +       X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X,    &idle_cpu_spr),
>         X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,        &idle_cpu_knl),
>         X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,        &idle_cpu_knl),
>         X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT,       &idle_cpu_bxt),
> --