[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 02/15] xen/x86: introduce new sub-hypercall to propagate CPPC data
On 28.03.2025 09:27, Penny, Zheng wrote: > [Public] > > Hi, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@xxxxxxxx> >> Sent: Tuesday, March 25, 2025 3:54 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 v3 02/15] xen/x86: introduce new sub-hypercall to >> propagate >> CPPC data >> >> On 25.03.2025 05:12, Penny, Zheng wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@xxxxxxxx> >>>> Sent: Monday, March 24, 2025 10:28 PM >>>> >>>> On 06.03.2025 09:39, Penny Zheng wrote: >>>>> + pm_info = processor_pminfo[cpuid]; >>>>> + /* Must already allocated in set_psd_pminfo */ >>>>> + if ( !pm_info ) >>>>> + { >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>>> + pm_info->cppc_data = *cppc_data; >>>>> + >>>>> + if ( cpufreq_verbose ) >>>>> + print_CPPC(&pm_info->cppc_data); >>>>> + >>>>> + pm_info->init = XEN_CPPC_INIT; >>>> >>>> That is - whichever Dom0 invoked last will have data recorded, and >>>> the other effectively is discarded? I think a warning (perhaps a >>>> one-time one) is minimally needed to diagnose the case where one type of >> data replaces the other. >>>> >>> >>> In last v2 discussion, we are discussing that either set_px_pminfo or >>> set_cppc_pminfo shall be invoked, which means either PX data is recorded, or >> CPPC data is recorded. >>> Current logic is that, cpufreq cmdline logic will set the >>> XEN_PROCESSOR_PM_PX/CPPC flag to reflect user preference, if user >>> defines the fallback option, like "cpufreq=amd-cppc,xen", we will have both >> XEN_PROCESSOR_PM_PX | XEN_PROCESSOR_PM_CPPC set in the >> beginning. >>> Later in cpufreq driver register logic, as only one register could be >>> registered , if amd-cppc being registered successfully, it will clear the >> XEN_PROCESSOR_PM_PX flag bit. >>> But if it fails to register, fallback scheme kicks off, we will try >>> the legacy P-states, in the mean time, clearing the >> XEN_PROCESSOR_PM_CPPC. >>> We are trying to make XEN_PROCESSOR_PM_PX and >> XEN_PROCESSOR_PM_CPPC >>> exclusive values after driver registration, which will ensure us that >>> either set_px_pminfo or set_cppc_pminfo is taken in the runtime. >> >> Yet you realize that this implies Dom0 to know what configuration Xen uses, >> in >> order to know which data to upload. The best approach might be to have >> Dom0 upload all data it has, with us merely ignoring what we can't make use >> of. > > PLZ correct me if I understand you wrongly: > Right now, I was letting DOM0 upload all data it has, and in the Xen: > ``` > case XEN_PM_CPPC: > if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) ) > { > ret = -EOPNOTSUPPED; > break; > } > ret = set_cppc_pminfo(op->u.set_pminfo.id, > &op->u.set_pminfo.u.cppc_data); > break; > > case XEN_PM_PX: > if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) ) > { > ret = -EOPNOTSUPPED; > break; > } > ret = set_px_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.perf); > break; > ``` > I relied on flag XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX to choose which > info we shall record. > Firstly, we shall not return -EOPNOTSUPPED error above there. Yes. >> The order of uploading (CPPC first or CPPC last) shouldn't matter. Then (and >> only >> then, and - ftaod - only when uploading of the "wrong" kind of data doesn't >> result in >> an error) things can go without warning. > > Then in > ``` > pm_info->init = XEN_CPPC_INIT; > ret = cpufreq_cpu_init(cpuid); > ``` > We shall add warning here to clarify no fallback scheme to replace now, when > ret is not zero. Maybe. In the earlier reply I said with certain conditions fulfilled a warning may not be necessary. Yet perhaps initially having a warning there (maybe just for debug builds) may make sense. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |