linux-kernel - Re: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to broadcast

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Mar 2019 10:34:46 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Chen Yu <yu.c.chen@...el.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Len Brown <len.brown@...el.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Yu Chen <yu.chen.surf@...il.com>
Subject: Re: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to
 broadcast _PPC

On Thu, Feb 28, 2019 at 11:18 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@...el.com> wrote:
> >
> > On some problematic platforms, the _PPC notifier is
> > only delivered to one CPU, which might cause information
> > inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this _PPC notifier onto all
> > online CPUs.
> >
> > Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> > ---
> >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index a303fd0e108c..737dbf5aa7f7 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> >                  "limited by BIOS, this should help");
> >
> > +static int broadcast_ppc;
> > +module_param(broadcast_ppc, int, 0644);
> > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > +
> >  #define PPC_REGISTERED   1
> >  #define PPC_IN_USE       2
> >
> > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> >                 else
> >                         acpi_processor_ppc_ost(pr->handle, 0);
> >         }
> > -       if (ret >= 0)
> > -               cpufreq_update_policy(pr->id);
> > +       if (ret >= 0) {
> > +               if (broadcast_ppc) {
> > +                       int cpu;
> > +
> > +                       for_each_possible_cpu(cpu)
> > +                               cpufreq_update_policy(cpu);
>
> This doesn't actually help AFAICS, because it only causes
> acpi_processor_ppc_notifier() to be called for all policies, but
> pr->performance_platform_limit is re-computed for the target CPU only
> anyway, so the limit will only be applied to that one.
>
> What happens in the BZ is that invoking cpufreq_update_policy() for
> all CPUs causes ->verify() to run on all of them which triggers
> update_turbo_state() and cpuinfo.max_freq update, because
> global.turbo_disabled has changed.
>
> That is rather less than straightforward and intel_pstate really
> doesn't need any _PPC change notifications to notice that the
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE bit has changed as it checks that
> bit on every P-state update.

Which admittedly may not be necessary if notifications are delivered.

I still don't think that updating all policies from
acpi_processor_ppc_has_changed() is a good idea, but yes, there is a
problem that if MSR_IA32_MISC_ENABLE_TURBO_DISABLE goes from set to
unset, all policies need to be updated to update policy->max
accordingly, so looking at MSR_IA32_MISC_ENABLE_TURBO_DISABLE from
within the driver without triggering a policy update is not
sufficient.