| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Oct 2021 03:41:28 +0000
From: "Huang, Ray" <Ray.Huang@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
Shuah Khan <skhan@...uxfoundation.org>,
Borislav Petkov <bp@...e.de>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>,
Linux PM <linux-pm@...r.kernel.org>,
"Sharma, Deepak" <Deepak.Sharma@....com>,
"Deucher, Alexander" <Alexander.Deucher@....com>,
"Limonciello, Mario" <Mario.Limonciello@....com>,
"Fontenot, Nathan" <Nathan.Fontenot@....com>,
"Su, Jinzhou (Joe)" <Jinzhou.Su@....com>,
"Du, Xiaojian" <Xiaojian.Du@....com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
the arch/x86 maintainers <x86@...nel.org>
Subject: RE: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
[AMD Official Use Only]
> -----Original Message-----
> From: Rafael J. Wysocki <rafael@...nel.org>
> Sent: Wednesday, October 20, 2021 9:32 PM
> To: Huang, Ray <Ray.Huang@....com>
> Cc: Rafael J. Wysocki <rafael@...nel.org>; Rafael J . Wysocki
> <rafael.j.wysocki@...el.com>; Viresh Kumar <viresh.kumar@...aro.org>;
> Shuah Khan <skhan@...uxfoundation.org>; Borislav Petkov <bp@...e.de>;
> Peter Zijlstra <peterz@...radead.org>; Ingo Molnar <mingo@...nel.org>;
> Linux PM <linux-pm@...r.kernel.org>; Sharma, Deepak
> <Deepak.Sharma@....com>; Deucher, Alexander
> <Alexander.Deucher@....com>; Limonciello, Mario
> <Mario.Limonciello@....com>; Fontenot, Nathan
> <Nathan.Fontenot@....com>; Su, Jinzhou (Joe) <Jinzhou.Su@....com>;
> Du, Xiaojian <Xiaojian.Du@....com>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; the arch/x86 maintainers <x86@...nel.org>
> Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
>
> On Wed, Oct 20, 2021 at 1:13 PM Huang, Ray <Ray.Huang@....com>
> wrote:
> >
> > [AMD Official Use Only]
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@...nel.org>
> > > Sent: Wednesday, October 20, 2021 1:00 AM
> > > To: Huang, Ray <Ray.Huang@....com>
> > > Cc: Rafael J . Wysocki <rafael.j.wysocki@...el.com>; Viresh Kumar
> > > <viresh.kumar@...aro.org>; Shuah Khan <skhan@...uxfoundation.org>;
> > > Borislav Petkov <bp@...e.de>; Peter Zijlstra <peterz@...radead.org>;
> > > Ingo Molnar <mingo@...nel.org>; Linux PM <linux-pm@...r.kernel.org>;
> > > Sharma, Deepak <Deepak.Sharma@....com>; Deucher, Alexander
> > > <Alexander.Deucher@....com>; Limonciello, Mario
> > > <Mario.Limonciello@....com>; Fontenot, Nathan
> > > <Nathan.Fontenot@....com>; Su, Jinzhou (Joe)
> <Jinzhou.Su@....com>;
> > > Du, Xiaojian <Xiaojian.Du@....com>; Linux Kernel Mailing List
> > > <linux- kernel@...r.kernel.org>; the arch/x86 maintainers
> > > <x86@...nel.org>
> > > Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register
> > > function
> > >
> > > On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <ray.huang@....com>
> wrote:
> > > >
> > > > From: Jinzhou Su <Jinzhou.Su@....com>
> > > >
> > > > Add a new function to enable CPPC feature. This function will
> > > > write Continuous Performance Control package EnableRegister field
> > > > on the processor.
> > >
> > > And what is going to take place after this write?
> > >
> > > Also, it would be good to mention that the user of this function
> > > will be added subsequently.
> >
> > After the enable flag is set, the processor hardware can accept the
> performance goals such as desired perf that programed by kernel and control
> the processor frequency according to the performance value.
>
> Is this the CPPC EnableRegister register described in Section 8.4.7.1 of ACPI
> 6.4? If so, it would be good to provide this information in the changelog
> either.
>
I see, yes. We should follow the spec definition for general CPPC function helper.
> > I will mention this in the comment in V3.
> >
> > >
> > > > Signed-off-by: Jinzhou Su <Jinzhou.Su@....com>
> > > > Signed-off-by: Huang Rui <ray.huang@....com>
> > > > ---
> > > > drivers/acpi/cppc_acpi.c | 48
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > > include/acpi/cppc_acpi.h | 5 +++++
> > > > 2 files changed, 53 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > > index
> > > > 2efe2ba97d96..b285960c35e7 100644
> > > > --- a/drivers/acpi/cppc_acpi.c
> > > > +++ b/drivers/acpi/cppc_acpi.c
> > > > @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > > > cppc_perf_fb_ctrs *perf_fb_ctrs) }
> > > > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> > > >
> > > > +/**
> > > > + * cppc_set_enable - Set to enable CPPC on the processor by
> > > > +writing the
> > > > + * Continuous Performance Control package EnableRegister feild.
> > > > + * @cpu: CPU for which to enable CPPC register.
> > > > + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> > > > + *
> > > > + * Return: 0 for success, -ERRNO or -EIO otherwise.
> > > > + */
> > > > +int cppc_set_enable(int cpu, u32 enable) {
> > > > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > > + struct cpc_register_resource *enable_reg;
> > > > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > > + struct cppc_pcc_data *pcc_ss_data = NULL;
> > > > + int ret = -1;
> > > > +
> > > > + /* check the input value*/
> > > > + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable >
> > > > + 1)
> > >
> > > Why not use cpu_possible()? And why enable > 1 is a problem?
> > >
> >
> > Yes, you're right, cpu_possible() is better here.
> > Will remove "enable > 1", and yes, we should support "disable" as well.
> >
> >
> > > > + return -ENODEV;
> > >
> > > -EINVAL
> > >
> >
> > Updated.
> >
> > > > +
> > > > + if (!cpc_desc) {
> > >
> > > if this is checked, the cpu_possible() check above is redundant.
> >
> > Hmm, if acpi_cppc_processor_probe got failed, some one outside acpi
> driver would like to call this helper.
> > Is that possible we get a null cpc descriptor here? Or anything I missed.
>
> if cpu_possible(cpu) is false, then cpc_desc for cpu will be NULL. If you check
> the latter, there's no need to check the former. Of course, cpc_desc may be
> NULL for other reasons, but you're checking it anyway.
>
Yes. If the cpc_desc is initialized, the cpu has to be in possible mask. I will clean it up in V3.
> > >
> > > > + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> > > > +
> > > > + if (CPC_IN_PCC(enable_reg)) {
> > > > +
> > > > + if (pcc_ss_id < 0)
> > > > + return -EIO;
> > > > +
> > > > + ret = cpc_write(cpu, enable_reg, enable);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pcc_ss_data = pcc_data[pcc_ss_id];
> > > > +
> > > > + down_write(&pcc_ss_data->pcc_lock);
> > > > + /* after writing CPC, transfer the ownership of PCC to
> platfrom */
> > > > + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > > + up_write(&pcc_ss_data->pcc_lock);
> > > > + }
> > >
> > > Does it really need to do nothing if the register is not in PCC? If so, then
> why?
> > >
> >
> > Hmm, do you mean we should take care the cases for enabling behavior if
> register in other spaces such as SYSTEM_MEMORY or FIXED_HARDWARE on
> different kinds of SBIOS implementation?
>
> This is a generic interface and it should cover all of the valid use cases, so yes.
I got it, thanks to reminder!
Thanks,
Ray
Powered by blists - more mailing lists