Re: [PATCH v7 06/13] xen/cpufreq: introduce new sub-hypercall to propagate CPPC data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2025 12:52, Penny Zheng wrote:
> --- a/xen/arch/x86/x86_64/cpufreq.c
> +++ b/xen/arch/x86/x86_64/cpufreq.c
> @@ -54,3 +54,22 @@ int compat_set_px_pminfo(uint32_t acpi_id,
>  
>      return set_px_pminfo(acpi_id, xen_perf);
>  }
> +
> +int compat_set_cppc_pminfo(unsigned int acpi_id,
> +                           const struct compat_processor_cppc *cppc_data)
> +
> +{
> +    struct xen_processor_cppc *xen_cppc;
> +    unsigned long xlat_page_current;
> +
> +    xlat_malloc_init(xlat_page_current);
> +
> +    xen_cppc = xlat_malloc_array(xlat_page_current,
> +                                 struct xen_processor_cppc, 1);
> +    if ( unlikely(xen_cppc == NULL) )
> +        return -EFAULT;

I think we want to avoid repeating the earlier mistake with using a wrong
error code. It's ENOMEM or ENOSPC or some such.

> --- a/xen/drivers/acpi/pm-op.c
> +++ b/xen/drivers/acpi/pm-op.c
> @@ -91,7 +91,9 @@ 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) ||

I fear I don't understand this: In the PX case we check whether necessary
data is lacking. In the CPPC case you check that some data was provided
that we don't want to use? Why not similarly check that data we need was
provided?

> @@ -693,6 +699,120 @@ 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%d (ACPI ID %u) CPPC state info:\n",
> +               cpuid, acpi_id);
> +
> +    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) &&
> +         !check_psd_pminfo(cppc_data->shared_type) )
> +    {
> +            ret = -EINVAL;
> +            goto out;

Indentation.

Jan