Re: [PATCH v6 10/19] xen/cpufreq: introduce new sub-hypercall to propagate CPPC data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2025 05:50, Penny Zheng wrote:
> --- a/xen/drivers/acpi/pm-op.c
> +++ b/xen/drivers/acpi/pm-op.c
> @@ -91,7 +91,8 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      pmpt = processor_pminfo[op->cpuid];
>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>  
> -    if ( !pmpt || !pmpt->perf.states ||
> +    if ( !pmpt || ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states) ||
> +         ((pmpt->init & XEN_CPPC_INIT) && pmpt->perf.state_count) ||

Nit: I think this would be neater if the PX_INIT part was also moved to its own
line.

> @@ -697,6 +703,122 @@ int acpi_set_pdc_bits(unsigned int acpi_id, 
> XEN_GUEST_HANDLE(uint32) pdc)
>      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 )
> +    {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    if ( cppc_data->pad[0] || cppc_data->pad[1] || cppc_data->pad[2] )
> +    {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    if ( cpufreq_verbose )
> +        printk("Set CPU acpi_id(%u) cpuid(%d) CPPC State info:\n",

May I suggest "Set CPU%d (ACPI ID %u) 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 ( cppc_data->flags & XEN_CPPC_PSD )
> +        if ( !check_psd_pminfo(cppc_data->shared_type) )

Please convert these into a single if().

> +        {
> +            ret = -EINVAL;
> +            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 ||
> +             cppc_data->cpc.lowest_nonlinear_perf >
> +                cppc_data->cpc.nominal_perf ||

Indentation is a little odd here. Best may be to use parentheses:

             cppc_data->cpc.lowest_perf > UINT8_MAX ||
             (cppc_data->cpc.lowest_perf >
              cppc_data->cpc.lowest_nonlinear_perf) ||
             (cppc_data->cpc.lowest_nonlinear_perf >
              cppc_data->cpc.nominal_perf) ||

Otherwise, strictly speaking, no extra indentation should be used. I can see
though that this would hamper readability, so the next best alternative would
appear to be to make the extra indentation a proper level (i.e. 4 blanks):

             cppc_data->cpc.lowest_perf > UINT8_MAX ||
             cppc_data->cpc.lowest_perf >
                 cppc_data->cpc.lowest_nonlinear_perf ||
             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 highest_perf/nominal_perf/
> +             * lowest_nonlinear_perf/lowest_perf values read from ACPI _CPC
> +             * table. Xen reads CPPC capability MSR to get these four values.
> +             * So warning is enough.
> +             */
> +            printk_once(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);
> +
> +        /* lowest_mhz and nominal_mhz are optional value */
> +        if ( cppc_data->cpc.lowest_mhz > cppc_data->cpc.nominal_mhz )

If they're optional, what if lowest_mhz is provided but nominal_mhz isn't?
Wouldn't the warning needlessly trigger in that case?

> +        {
> +            printk_once(XENLOG_WARNING
> +                        "Broken CPPC freq values: lowest(%u), nominal(%u)\n",
> +                        cppc_data->cpc.lowest_mhz,
> +                        cppc_data->cpc.nominal_mhz);
> +            /* Re-set with zero values, instead of keeping invalid values */
> +            pm_info->cppc_data.cpc.nominal_mhz = 0;
> +            pm_info->cppc_data.cpc.lowest_mhz = 0;
> +        }
> +    }
> +
> +    if ( cppc_data->flags == (XEN_CPPC_PSD | XEN_CPPC_CPC) )
> +    {
> +        if ( cpufreq_verbose )
> +        {
> +            print_PSD(&pm_info->cppc_data.domain_info);
> +            print_CPPC(&pm_info->cppc_data);
> +        }
> +
> +        pm_info->init = XEN_CPPC_INIT;
> +        ret = cpufreq_cpu_init(cpuid);
> +        if ( ret )
> +            printk_once(XENLOG_WARNING
> +                        "CPU%u failed amd-cppc mode init; use 
> \"cpufreq=xen\" instead",
> +                        cpuid);

cpuid is still int, so wants printing with %d.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -363,6 +363,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
>  #define XEN_PM_PX   1
>  #define XEN_PM_TX   2
>  #define XEN_PM_PDC  3
> +#define XEN_PM_CPPC 4
>  
>  /* Px sub info type */
>  #define XEN_PX_PCT   1
> @@ -370,6 +371,10 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
>  #define XEN_PX_PPC   4
>  #define XEN_PX_PSD   8
>  
> +/* CPPC sub info type */
> +#define XEN_CPPC_PSD   1
> +#define XEN_CPPC_CPC   2

As per this, ...

> @@ -457,6 +462,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; /* IN: XEN_CPPC_xxx */

... it's a type that's living here, not a collection of flags. Any reason the
field isn't named "type"?

> +    uint8_t pad[3];
> +    /*
> +     * IN: 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;

What, again, was the reason to wrap these into a sub-struct?

Jan