Re: [PATCH v2 02/11] xen/x86: introduce new sub-hypercall to propagate CPPC data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2025 09:32, Penny Zheng wrote:
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -572,6 +572,10 @@ ret_t do_platform_op(
>              break;
>          }
>  
> +        case XEN_PM_CPPC:
> +            ret = set_cppc_pminfo(op->u.set_pminfo.id, 
> &op->u.set_pminfo.u.cppc_data);

Nit: Too long line.

> --- a/xen/arch/x86/x86_64/cpufreq.c
> +++ b/xen/arch/x86/x86_64/cpufreq.c
> @@ -26,6 +26,8 @@
>  #include <xen/pmstat.h>
>  #include <compat/platform.h>
>  
> +CHECK_processor_cppc;
> +
>  CHECK_processor_px;

May I ask that you insert below the pre-existing CHECK_* rather than above?
Or wait - maybe you were aiming at sorting these alphabetically? That would
perhaps be fine then.

> @@ -458,6 +459,53 @@ static void print_PPC(unsigned int platform_limit)
>      printk("\t_PPC: %d\n", platform_limit);
>  }
>  
> +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_freq=%uMhz, lowest_freq=%uMhz\n",

Nit: MHz please.

> +           cppc_data->highest_perf, cppc_data->lowest_perf,
> +           cppc_data->nominal_perf, cppc_data->lowest_nonlinear_perf,
> +           cppc_data->nominal_freq, cppc_data->lowest_freq);
> +}
> +
> +int set_cppc_pminfo(uint32_t acpi_id, const struct xen_processor_cppc 
> *cppc_data)

Too long a line again.

Also while print_CPPC() is placed okay, this function wants to move
down, past set_px_pminfo().

> +{
> +    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(%d) 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;
> +    pm_info->cppc_data = *cppc_data;
> +
> +    if ( cpufreq_verbose )
> +        print_CPPC(&pm_info->cppc_data);
> +
> + out:
> +    return ret;
> +}

What's the interaction between the data set by set_px_pminfo() and the
data set here? In particular, what's going to happen if both functions
come into play for the same CPU? Shouldn't there be some sanity checks?
Will consumers be able to tell which of the two were correctly invoked,
before using respective data? Even in the event of no code changes at
all to address this, it will want discussing in the patch description.

> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -5,6 +5,7 @@
>  #include <public/platform.h> /* for struct xen_processor_power */
>  #include <public/sysctl.h>   /* for struct pm_cx_stat */
>  
> +int set_cppc_pminfo(uint32_t cpu, const struct xen_processor_cppc 
> *cppc_data);

Surprisingly this line is within limits, when the (supposedly) one char
shorter line at the definition site is not. Which points out a Misra
violation: Declaration and definition ought to agree in parameter names.

Jan