[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, February 18, 2025 10:49 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 18.02.2025 07:05, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Monday, February 17, 2025 3:39 PM
> >>
> >> 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.
>
> You did read this, didn't you? I ask because the same boilerplate keeps
> appearing in
> your mails.
>
> >>>> -----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_Contr
> >>> ol
> >>> .html?highlight=cppc#power-performance-and-throttling-state-dependen
> >>> ci 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.Zheng@amd.c
> > om/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?
>
> Value range checking is nice to have in Dom0, but the same checking needs
> doing
> in the hypervisor anyway. But that isn't what my comment was about. What I'm
> asking is how it is being made sure that we won't have to deal with a mix of
> traditional and CPPC data in the hypervisor.
>
Are you suggesting that we only do either set_cppc_pminfo or set_px_pminfo?
Only one side data get set to avoid the consequence of mixture.
> Jan
