[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, February 17, 2025 3:39 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andryuk, Jason
> <jason.andr...@amd.com>; Andrew Cooper <andrew.coop...@citrix.com>;
> Roger Pau Monné <roger....@citrix.com>; Anthony PERARD
> <anthony.per...@vates.tech>; Orzel, Michal <michal.or...@amd.com>; Julien
> Grall <jul...@xen.org>; Stefano Stabellini <sstabell...@kernel.org>; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v2 02/11] xen/x86: introduce new sub-hypercall to
> propagate
> CPPC data
>
> On 17.02.2025 08:20, Penny, Zheng wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
>
> Btw, boiler plates like this aren't really liked on public mailing lists, for
> being contrary
> to the purpose of such lists.
>
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Tuesday, February 11, 2025 7:10 PM
> >>
> >> On 06.02.2025 09:32, Penny Zheng wrote:
> >>> +{
> >>> + 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-dependenci
> > es 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?
>
> Maybe, but that wasn't the point of my remark.
>
> Properly behaving Dom0 should probably be passing only one of the two possible
> pieces of information. Yet maybe we'd better sanity check _that_?
> (I don't recall seeing Linux kernel side patches yet; if they were posted
> somewhere,
> they may at least partly address my concern.)
>
In my linux patch,
https://lore.kernel.org/lkml/20241204082430.469092-1-penny.zh...@amd.com/T/
I only did zero-value check in xen_processor_get_perf_caps(), Do you think in
that place, I shall add
more strict sanity check, like the register value shall not be zero and also
must smaller than UINT8_T?
Or we just do the above check in Xen part when receiving the data?
> >> 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?
>
> Mentioning this in the description would help. Yet the processor ignoring the
> other P-
> state control interface shouldn't mean we can nevertheless try to drive it.
> It has to
> be clear (and at least halfway obvious) internally to Xen that we only ever
> use one
> of the two. My present reading of the patches suggests that this is all
> implicit (and
> maybe not even guaranteed) right now.
Understood!
>
> Jan
