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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 09:11, Penny Zheng wrote:
> In order to provide backward compatibility with existing governors
> that represent performance as frequencies, like ondemand, the _CPC
> table can optionally provide processor frequency range values, Lowest
> frequency and Norminal frequency, to let OS use Lowest Frequency/
> Performance and Nominal Frequency/Performance as anchor points to
> create linear mapping of CPPC abstract performance to CPU frequency.
> 
> As Xen is uncapable of parsing the ACPI dynamic table, this commit
> introduces a new sub-hypercall to get required CPPC data from
> dom0 kernel.

"get" as used both here and in the title is, to me, something an entity
does actively. Xen is entirely passive here, though. (Reading the title
I was first assuming this is about a sub-op to get certain data out of
Xen.)

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -572,6 +572,12 @@ 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);
> +        }
> +        break;

No such unnecessary figure braces please, which - once dropped - will
also call for "break" to be indented one level deeper.

> --- a/xen/arch/x86/x86_64/cpufreq.c
> +++ b/xen/arch/x86/x86_64/cpufreq.c
> @@ -54,3 +54,21 @@ int compat_set_px_pminfo(uint32_t acpi_id,
>  
>      return set_px_pminfo(acpi_id, xen_perf);
>  }
> +
> +int compat_set_cppc_pminfo(uint32_t acpi_id,
> +                           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;
> +
> +    XLAT_processor_cppc(xen_cppc, cppc_data);
> +
> +    return set_cppc_pminfo(acpi_id, xen_cppc);
> +}

Why's this needed? The structure - for now at least - consists of only
uint32_t-s, and hence has identical layout for compat callers.

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -458,6 +458,56 @@ static void print_PPC(unsigned int platform_limit)
>      printk("\t_PPC: %d\n", platform_limit);
>  }
>  
> +static void print_CPPC(struct xen_processor_cppc *cppc_data)

Pointer-to-const?

> +{
> +    printk("\t_CPC: highest_perf=%u, lowest_perf=%u, "
> +           "nominal_perf=%u, lowest_nonlinear_perf=%u, "
> +           "nominal_freq=%uMhz, lowest_freq=%uMhz\n",
> +           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, struct xen_processor_cppc *cppc_data)

Pointer-to-const?

> +{
> +    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 = xzalloc(struct processor_pminfo);

Please be aware that new code is supposed to be using xvmalloc().

> +        if ( !pm_info )
> +        {
> +            ret = -ENOMEM;
> +            goto out;
> +        }
> +        processor_pminfo[cpuid] = pm_info;
> +    }
> +    pm_info->acpi_id = acpi_id;
> +    pm_info->id = cpuid;
> +
> +    memcpy ((void *)&pm_info->cppc_data,
> +            (void *)cppc_data,
> +            sizeof(struct xen_processor_cppc));

What use are these casts? Also please no blank before the opening parenthesis
of a function call, and please sizeof(*cppc_data). Yet then - why memcpy() in
the first place? This can be a (type safe) structure assignment, can't it?

> --- 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
> @@ -432,6 +433,15 @@ struct xen_processor_px {
>  typedef struct xen_processor_px xen_processor_px_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_processor_px_t);
>  
> +struct xen_processor_cppc {
> +    uint32_t highest_perf;
> +    uint32_t nominal_perf;
> +    uint32_t lowest_perf;
> +    uint32_t lowest_nonlinear_perf;
> +    uint32_t lowest_freq;
> +    uint32_t nominal_freq;
> +};

_CPC contains a lot more data. Please clarify on what basis this subset was
chosen. (Keeping the chosen fields in the order _CPC has them might also be
a good idea.)

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

Please obey to alphabetic sorting. As per an earlier comment I also expect
this wants to be using '?' in place of '!'.

Jan