Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
> From: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> 
> Add C-state table for the SnowRidge SoC which is found on Intel Jacobsville
> platforms.
> 
> The following has been changed.
> 
>  1. C1E latency changed from 10us to 15us. It was measured using the
>     open source "wult" tool (the "nic" method, 15us is the 99.99th
>     percentile).
> 
>  2. C1E power break even changed from 20us to 25us, which may result
>     in less C1E residency in some workloads.
> 
>  3. C6 latency changed from 50us to 130us. Measured the same way as C1E.
> 
> The C6 C-state is supported only by some SnowRidge revisions, so add a C-state
> table commentary about this.
> 
> On SnowRidge, C6 support is enumerated via the usual mechanism: "mwait" leaf 
> of
> the "cpuid" instruction. The 'intel_idle' driver does check this leaf, so even
> though C6 is present in the table, the driver will only use it if the CPU does
> support it.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> [Linux commit: 9cf93f056f783f986c19f40d5304d1bcffa0fc0d]
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
>       {}
>  };
>  
> +/*
> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not 
> support
> + * C6, and this is indicated in the CPUID mwait leaf.
> + */
> +static const struct cpuidle_state snr_cstates[] = {
> +     {
> +             .name = "C1",

We usually use names like "C1-SNR" or similar in other cpuidle_state
structs. Is using plain "C1" intentional here?

> +             .flags = MWAIT2flg(0x00),
> +             .exit_latency = 2,
> +             .target_residency = 2,
> +     },
> +     {
> +             .name = "C1E",
> +             .flags = MWAIT2flg(0x01),
> +             .exit_latency = 15,
> +             .target_residency = 25,
> +     },
> +     {
> +             .name = "C6",
> +             .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
> +             .exit_latency = 130,
> +             .target_residency = 500,
> +     },
> +     {}
> +};
> +
>  static void mwait_idle(void)
>  {
>       unsigned int cpu = smp_processor_id();
> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
>       .disable_promotion_to_c1e = 1,
>  };
>  
> +static const struct idle_cpu idle_cpu_snr = {
> +     .state_table = snr_cstates,
> +     .disable_promotion_to_c1e = true,

This likely wants to be 1 because the type is bool_t.

Thanks, Roger.