Re: [PATCH v4 03/15] xen/x86: introduce new sub-hypercall to propagate CPPC data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> In order to provide backward compatibility with existing governors
> that represent performance as frequency, like ondemand, the _CPC
> table can optionally provide processor frequency range values, Lowest
> frequency and Norminal frequency, to let OS use Lowest Frequency/

Nit: Nominal

> @@ -497,12 +504,19 @@ static void print_PPC(unsigned int platform_limit)
>      printk("\t_PPC: %d\n", platform_limit);
>  }
>  
> -static int check_psd_pminfo(const struct xen_processor_performance *perf)
> +static int check_psd_pminfo(const struct xen_processor_performance *perf,
> +                            const struct xen_processor_cppc *cppc_data)
>  {
> +    uint32_t shared_type;
> +
> +    if ( !perf && !cppc_data )
> +        return -EINVAL;
> +
> +    shared_type = perf ? perf->shared_type : cppc_data->shared_type;

Why don't you have the caller pass in shared_type? The two pointers aren't
used ...

>      /* check domain coordination */
> -    if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> -         perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> -         perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
> +    if ( shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> +         shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> +         shared_type != CPUFREQ_SHARED_TYPE_HW )
>          return -EINVAL;
>  
>      return 0;

... for anything else.

> @@ -627,6 +641,109 @@ out:
>      return ret;
>  }
>  
> +static void print_CPPC(const struct xen_processor_cppc *cppc_data)
> +{
> +    printk("\t_CPC: highest_perf=%u, lowest_perf=%u, "
> +           "nominal_perf=%u, lowest_nonlinear_perf=%u, "
> +           "nominal_mhz=%uMHz, lowest_mhz=%uMHz\n",
> +           cppc_data->cpc.highest_perf, cppc_data->cpc.lowest_perf,
> +           cppc_data->cpc.nominal_perf, cppc_data->cpc.lowest_nonlinear_perf,
> +           cppc_data->cpc.nominal_mhz, cppc_data->cpc.lowest_mhz);
> +}
> +
> +int set_cppc_pminfo(unsigned int acpi_id,
> +                    const struct xen_processor_cppc *cppc_data)
> +{
> +    int ret = 0, cpuid;
> +    struct processor_pminfo *pm_info;
> +
> +    cpuid = get_cpu_id(acpi_id);
> +    if ( cpuid < 0 || !cppc_data )
> +    {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +    if ( cpufreq_verbose )
> +        printk("Set CPU acpi_id(%u) cpuid(%d) CPPC State info:\n",
> +               acpi_id, cpuid);
> +
> +    pm_info = processor_pminfo[cpuid];
> +    if ( !pm_info )
> +    {
> +        pm_info = xvzalloc(struct processor_pminfo);
> +        if ( !pm_info )
> +        {
> +            ret = -ENOMEM;
> +            goto out;
> +        }
> +        processor_pminfo[cpuid] = pm_info;
> +    }
> +    pm_info->acpi_id = acpi_id;
> +    pm_info->id = cpuid;
> +
> +    if ( cppc_data->flags & XEN_CPPC_PSD )
> +    {
> +        ret = check_psd_pminfo(NULL, cppc_data);
> +        if ( ret )
> +            goto out;
> +    }
> +
> +    if ( cppc_data->flags & XEN_CPPC_CPC )
> +    {
> +        if ( cppc_data->cpc.highest_perf == 0 ||
> +             cppc_data->cpc.highest_perf > UINT8_MAX ||
> +             cppc_data->cpc.nominal_perf == 0 ||
> +             cppc_data->cpc.nominal_perf > UINT8_MAX ||
> +             cppc_data->cpc.lowest_nonlinear_perf == 0 ||
> +             cppc_data->cpc.lowest_nonlinear_perf > UINT8_MAX ||
> +             cppc_data->cpc.lowest_perf == 0 ||
> +             cppc_data->cpc.lowest_perf > UINT8_MAX ||
> +             cppc_data->cpc.lowest_perf >
> +                cppc_data->cpc.lowest_nonlinear_perf ||

Where's this ordering spelled out in the spec?

> +             cppc_data->cpc.lowest_nonlinear_perf >
> +                cppc_data->cpc.nominal_perf ||
> +             cppc_data->cpc.nominal_perf > cppc_data->cpc.highest_perf )
> +            /*
> +             * Right now, Xen doesn't actually use perf values
> +             * in ACPI _CPC table, warning is enough.
> +             */
> +            printk(XENLOG_WARNING
> +                   "Broken CPPC perf values: lowest(%u), 
> nonlinear_lowest(%u), nominal(%u), highest(%u)\n",
> +                   cppc_data->cpc.lowest_perf,
> +                   cppc_data->cpc.lowest_nonlinear_perf,
> +                   cppc_data->cpc.nominal_perf,
> +                   cppc_data->cpc.highest_perf);

If this warning was to ever surface, it would likely surface for every CPU.
That's unnecessarily verbose, I guess. Please consider using printk_once()
here.

Also, is "right now" (as the comment says) still going to be true by the
end of the series? Didn't I see you use the values in earlier versions?

> +        /* lowest_mhz and nominal_mhz are optional value */
> +        if ( (cppc_data->cpc.lowest_mhz && cppc_data->cpc.nominal_mhz) &&
> +             cppc_data->cpc.lowest_mhz > cppc_data->cpc.nominal_mhz )

The 1st of the three checks is redundant with the 3rd one. There's also no
point parenthesizing one && against another.

> +            printk(XENLOG_WARNING
> +                   "Broken CPPC freq values: lowest(%u), nominal(%u)\n",
> +                   cppc_data->cpc.lowest_mhz,
> +                   cppc_data->cpc.nominal_mhz);
> +    }
> +
> +    if ( cppc_data->flags == (XEN_CPPC_PSD | XEN_CPPC_CPC) )

If either flag may be clear, ...

> +    {
> +        pm_info->cppc_data = *cppc_data;
> +        if ( cpufreq_verbose )
> +        {
> +            print_PSD(&pm_info->cppc_data.domain_info);
> +            print_CPPC(&pm_info->cppc_data);

... why unconditionally loog both?

> +        }
> +
> +        pm_info->init = XEN_CPPC_INIT;

Plus is it correct to set this flag if either of the incoming flags was clear?

> +        ret = cpufreq_cpu_init(cpuid);
> +#ifndef NDEBUG

Instead of this, ...

> +        if ( ret )
> +            printk(XENLOG_WARNING "No fallback scheme could be replaced 
> now");

... did you perhaps mean to use dprintk()? Also, the wording isn't meaningful
at all. Seeing the message, about everyone will need to go and find the text
in source code in order to stand a chance of gaining even basic understanding
of what's going on.

> @@ -459,6 +464,26 @@ struct xen_processor_performance {
>  typedef struct xen_processor_performance xen_processor_performance_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
>  
> +struct xen_processor_cppc {
> +    uint8_t flags; /* flag for CPPC sub info type */
> +    /*
> +     * Subset _CPC fields useful for CPPC-compatible cpufreq
> +     * driver's initialization
> +     */
> +    struct {
> +        uint32_t highest_perf;
> +        uint32_t nominal_perf;
> +        uint32_t lowest_nonlinear_perf;
> +        uint32_t lowest_perf;
> +        uint32_t lowest_mhz;
> +        uint32_t nominal_mhz;
> +    } cpc;
> +    struct xen_psd_package domain_info; /* _PSD */

This being a member of the new type, ...

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -168,6 +168,7 @@
>  !    processor_performance           platform.h
>  !    processor_power                 platform.h
>  ?    processor_px                    platform.h
> +?    processor_cppc                  platform.h

... how can it be ? here when it's ...

>  !    psd_package                     platform.h

... ! here? And with it being ?, you're lacking a place where you invoke the
resulting checking macro (which I assume would cause a build failure).

Also when laying out struct xen_processor_cppc, please avoid unnecessary
gaps or tail padding - it looks like "shared_type" would better move up. I
think it would also be a good idea to make padding fields explicit, and
check them to be zero. This way they can be assigned meaning later (if need
be) without breaking backwards compatibility.

Jan