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

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, August 26, 2025 1:59 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 06/13] xen/cpufreq: introduce new sub-hypercall to
> propagate CPPC data
>
> On 26.08.2025 07:53, Penny, Zheng wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Monday, August 25, 2025 11:02 PM
> >> To: Penny, Zheng <penny.zheng@xxxxxxx>
> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> >> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> >> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> >> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> >> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v7 06/13] xen/cpufreq: introduce new
> >> sub-hypercall to propagate CPPC data
> >>
> >> 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.
> >>
> >
> > Understood, I'll change it to -ENOMEM
> >
> >>> --- 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?
> >>
> >
> > We are introducing another checking line for CPPC is actually to avoid NULL
> deref of state[i]:
> > ```
> >         for ( i = 0; i < op->u.get_para.freq_num; i++ )
> >                 data[i] = pmpt->perf.states[i].core_frequency * 1000;
> > ``` We want to ensure "op->u.get_para.freq_num" is always zero in CPPC
> > mode, which is validated against pmpt->perf.state_count.
> > We have similar discussion in here
> > https://old-list-archives.xen.org/archives/html/xen-devel/2025-06/msg0
> > 1160.html
>
> Indeed I was thinking that we would have touched this before. As to your 
> reply:
> This explains the .state_count check (which imo wants a comment). It doesn't,

Understood, I'll complement

> however, explain the absence of a "have we got the data we need" part. Unless 
> of
> course there simply isn't anything to check for.
>

Yes, imo, there isn’t anything to check.
In get_cpufreq_para(). we are not accessing data specific to CPPC.

> Jan