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

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, February 11, 2025 7:10 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> <Jason.Andryuk@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 v2 02/11] xen/x86: introduce new sub-hypercall to 
> propagate
> CPPC data
>
> On 06.02.2025 09:32, Penny Zheng wrote:
> > --- a/xen/arch/x86/platform_hypercall.c
> > +++ b/xen/arch/x86/platform_hypercall.c
> > @@ -572,6 +572,10 @@ 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);
>
> Nit: Too long line.
>
> > --- a/xen/arch/x86/x86_64/cpufreq.c
> > +++ b/xen/arch/x86/x86_64/cpufreq.c
> > @@ -26,6 +26,8 @@
> >  #include <xen/pmstat.h>
> >  #include <compat/platform.h>
> >
> > +CHECK_processor_cppc;
> > +
> >  CHECK_processor_px;
>
> May I ask that you insert below the pre-existing CHECK_* rather than above?
> Or wait - maybe you were aiming at sorting these alphabetically? That would
> perhaps be fine then.
>

Yes, I intended to sort these alphabetically

> > +{
> > +    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 = 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 ( cpufreq_verbose )
> > +        print_CPPC(&pm_info->cppc_data);
> > +
> > + out:
> > +    return ret;
> > +}
>
> What's the interaction between the data set by set_px_pminfo() and the data 
> set
> here? In particular, what's going to happen if both functions come into play 
> for the
> same CPU? Shouldn't there be some sanity checks?

Yes, I've considered this and checked ACPI spec. I'll refer them here:
```
If the platform supports CPPC, the _CPC object must exist under all processor 
objects.
That is, OSPM is not expected to support mixed mode (CPPC & legacy PSS, _PCT, 
_PPC) operation.
```
See 
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html?highlight=cppc#power-performance-and-throttling-state-dependencies
So CPUs could have both _CPC and legacy P-state info in ACPI for each entry, 
they just can't have mixed-mode
Maybe we shall add sanity check to see if _CPC exists, it shall exist for all 
pcpus?

> Will consumers be able to tell which of the two were correctly invoked, 
> before using
> respective data? Even in the event of no code changes at all to address this, 
> it will
> want discussing in the patch description.
>

I have checked the relevant spec. it shall be the following logic:
```
Software enables Collaborative Processor Performance Control by setting
CPPC_ENABLE[CPPC_En] (bit 0) = 1. Once it gets enabled, the processor ignores 
the legacy P-state control interface.
```
Hmmm, I shall add relevant comment in Doc?

>
> Jan

Many thanks,
Penny