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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, January 9, 2025 5:46 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Ragiadakou, Xenia <Xenia.Ragiadakou@xxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 02/11] xen/x86: introduce new sub-hypercall to get CPPC
> data
>
> 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.)

How about "a new sub-hypercall to pass/deliver CPPC to Xen"? or any better 
suggestions?

>
> > --- 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.

Understood.

>
> > --- 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.
>

Understood. Not familiar with the compat framework

> > --- 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?
>

Sure.

> > +{
> > +    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?
>

Sure.

> > +{
> > +    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().

Thanks for the update.

>
> > +        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?
>

Yes, it can be a (type safe) structure assignment

> > --- 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.)
>

I'll comment with "
/* Subset fields useful for CPPC-compatible cpufreq driver's initialization */
"
I'll stay the same order with the _CPC has them

> > --- 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 '!'.
>

Sure.

> Jan

Many thanks
Penny