Print HWP-specific parameters.  Some are always present, but others
depend on hardware support.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v2:
Style fixes
Declare i outside loop
Replace repearted hardware/configured limits with spaces
Fixup for hw_ removal
Use XEN_HWP_GOVERNOR
Use HWP_ACT_WINDOW_EXPONENT_*
Remove energy_perf hw autonomous - 0 doesn't mean autonomous

v4:
Return activity_window from calculate_hwp_activity_window
Use blanks instead of _ in output
Use MASK_EXTR
Check XEN_HWP_DRIVER name since governor is no longer returned
s/hwp/cppc

v5:
Add Jan's Reviewed-by
---
 tools/misc/xenpm.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 21c93386de..3abd99fd20 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -708,6 +708,46 @@ void start_gather_func(int argc, char *argv[])
     pause();
 }
 
+static unsigned int calculate_activity_window(const xc_cppc_para_t *cppc,
+                                              const char **units)
+{
+    unsigned int mantissa = MASK_EXTR(cppc->activity_window,
+                                      XEN_CPPC_ACT_WINDOW_MANTISSA_MASK);
+    unsigned int exponent = MASK_EXTR(cppc->activity_window,
+                                      XEN_CPPC_ACT_WINDOW_EXPONENT_MASK);
+    unsigned int multiplier = 1;
+    unsigned int i;
+
+    /*
+     * SDM only states a 0 register is hardware selected, and doesn't mention
+     * a 0 mantissa with a non-0 exponent.  Only special case a 0 register.
+     */
+    if ( cppc->activity_window == 0 )
+    {
+        *units = "hardware selected";
+
+        return 0;
+    }
+
+    if ( exponent >= 6 )
+    {
+        *units = "s";
+        exponent -= 6;
+    }
+    else if ( exponent >= 3 )
+    {
+        *units = "ms";
+        exponent -= 3;
+    }
+    else
+        *units = "us";
+
+    for ( i = 0; i < exponent; i++ )
+        multiplier *= 10;
+
+    return mantissa * multiplier;
+}
+
 /* print out parameters about cpu frequency */
 static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
 {
@@ -772,6 +812,32 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
                p_cpufreq->u.s.scaling_min_freq,
                p_cpufreq->u.s.scaling_cur_freq);
     }
+    else
+    {
+        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
+
+        printf("cppc variables       :\n");
+        printf("  hardware limits    : lowest [%u] lowest nonlinear [%u]\n",
+               cppc->lowest, cppc->lowest_nonlinear);
+        printf("                     : nominal [%u] highest [%u]\n",
+               cppc->nominal, cppc->highest);
+        printf("  configured limits  : min [%u] max [%u] energy perf [%u]\n",
+               cppc->minimum, cppc->maximum, cppc->energy_perf);
+
+        if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW )
+        {
+            unsigned int activity_window;
+            const char *units;
+
+            activity_window = calculate_activity_window(cppc, &units);
+            printf("                     : activity_window [%u %s]\n",
+                   activity_window, units);
+        }
+
+        printf("                     : desired [%u%s]\n",
+               cppc->desired,
+               cppc->desired ? "" : " hw autonomous");
+    }
 
     printf("turbo mode           : %s\n",
            p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a");
-- 
2.41.0

On Wed, Jul 26, 2023 at 01:09:41PM -0400, Jason Andryuk wrote:
> @@ -772,6 +812,32 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
>                 p_cpufreq->u.s.scaling_min_freq,
>                 p_cpufreq->u.s.scaling_cur_freq);
>      }
> +    else
> +    {

I feel like this could be confusing. In this function, we have both:
    if ( hwp ) { this; } else { that; }
and
    if ( !hwp ) { that; } else { this; }

If we could have the condition in the same order, or use the same
condition for both "true" blocks, that would be nice.


> +        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
> +
> +        printf("cppc variables       :\n");
> +        printf("  hardware limits    : lowest [%u] lowest nonlinear [%u]\n",
> +               cppc->lowest, cppc->lowest_nonlinear);

All these %u should be %"PRIu32", right? Even if the rest of the
function is bogus... and even if it's probably be a while before %PRIu32
is different from %u.

Thanks,

-- 
Anthony PERARD

On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Wed, Jul 26, 2023 at 01:09:41PM -0400, Jason Andryuk wrote:
> > @@ -772,6 +812,32 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
> >                 p_cpufreq->u.s.scaling_min_freq,
> >                 p_cpufreq->u.s.scaling_cur_freq);
> >      }
> > +    else
> > +    {
>
> I feel like this could be confusing. In this function, we have both:
>     if ( hwp ) { this; } else { that; }
> and
>     if ( !hwp ) { that; } else { this; }
>
> If we could have the condition in the same order, or use the same
> condition for both "true" blocks, that would be nice.

Makes sense.  From your comment on patch 8, I was thinking of
switching to a bool scaling_governor and using that.  But now I think
hwp is better since it's the specific governor - not the default one.
In light of that, I would just re-organize everything to be "if ( hwp
)".

With that, patch 8 is more of a transitive step, and I would leave the
"if ( !hwp )".  Then here in patch 11 the HWP code would be added
first inside "if ( hwp )".  Does that sound good?

> > +        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
> > +
> > +        printf("cppc variables       :\n");
> > +        printf("  hardware limits    : lowest [%u] lowest nonlinear [%u]\n",
> > +               cppc->lowest, cppc->lowest_nonlinear);
>
> All these %u should be %"PRIu32", right? Even if the rest of the
> function is bogus... and even if it's probably be a while before %PRIu32
> is different from %u.

Yes, you are correct.  In patch 8 "xenpm: Change get-cpufreq-para
output for hwp", some existing %u-s are moved and a few more added.
Do you want all of those converted to %PRIu32?

Thanks,
Jason

On Thu, Jul 27, 2023 at 09:54:17AM -0400, Jason Andryuk wrote:
> On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD
> <anthony.perard@citrix.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 01:09:41PM -0400, Jason Andryuk wrote:
> > > @@ -772,6 +812,32 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
> > >                 p_cpufreq->u.s.scaling_min_freq,
> > >                 p_cpufreq->u.s.scaling_cur_freq);
> > >      }
> > > +    else
> > > +    {
> >
> > I feel like this could be confusing. In this function, we have both:
> >     if ( hwp ) { this; } else { that; }
> > and
> >     if ( !hwp ) { that; } else { this; }
> >
> > If we could have the condition in the same order, or use the same
> > condition for both "true" blocks, that would be nice.
> 
> Makes sense.  From your comment on patch 8, I was thinking of
> switching to a bool scaling_governor and using that.  But now I think
> hwp is better since it's the specific governor - not the default one.
> In light of that, I would just re-organize everything to be "if ( hwp
> )".
> 
> With that, patch 8 is more of a transitive step, and I would leave the
> "if ( !hwp )".  Then here in patch 11 the HWP code would be added
> first inside "if ( hwp )".  Does that sound good?

Yes, that sounds fine.

> > > +        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
> > > +
> > > +        printf("cppc variables       :\n");
> > > +        printf("  hardware limits    : lowest [%u] lowest nonlinear [%u]\n",
> > > +               cppc->lowest, cppc->lowest_nonlinear);
> >
> > All these %u should be %"PRIu32", right? Even if the rest of the
> > function is bogus... and even if it's probably be a while before %PRIu32
> > is different from %u.
> 
> Yes, you are correct.  In patch 8 "xenpm: Change get-cpufreq-para
> output for hwp", some existing %u-s are moved and a few more added.
> Do you want all of those converted to %PRIu32?

For the newly added %u, yes, that would be nice.

As for the existing one, maybe as a separated patch, if you wish. At the
moment, patch 8 is easy to review with "--ignore-space-change".

Cheers,

-- 
Anthony PERARD