linux-kernel - Re: [PATCH v3 3/5] acpi: Introduce new function callback for

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]
Message-ID: <CAJZ5v0j945mvukeBkYzT=twbz2tagtMUSWgZkAfvhEkeGOHMeQ@mail.gmail.com>
Date:   Mon, 3 Jul 2023 17:20:32 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Wilczynski, Michal" <michal.wilczynski@...el.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-acpi@...r.kernel.org, andriy.shevchenko@...el.com,
        artem.bityutskiy@...ux.intel.com, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, lenb@...nel.org,
        jgross@...e.com, linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v3 3/5] acpi: Introduce new function callback for _OSC

On Mon, Jul 3, 2023 at 10:54 AM Wilczynski, Michal
<michal.wilczynski@...el.com> wrote:
>
>
>
> On 6/30/2023 10:46 AM, Wilczynski, Michal wrote:
> > Hi,
> > Thanks for the review !
> >
> > On 6/29/2023 1:04 PM, Rafael J. Wysocki wrote:
> >> I would just say "Introduce acpi_processor_osc()" in the subject and
> >> then explain its role in the changelog.
> > Sure,
> >
> >> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
> >> <michal.wilczynski@...el.com> wrote:
> >>> Currently in ACPI code _OSC method is already used for workaround
> >>> introduced in commit a21211672c9a ("ACPI / processor: Request native
> >>> thermal interrupt handling via _OSC"). Create new function, similar to
> >>> already existing acpi_hwp_native_thermal_lvt_osc(). Call new function
> >>> acpi_processor_osc(). Make this function fulfill the purpose previously
> >>> fulfilled by the workaround plus convey OSPM processor capabilities
> >>> with it by setting correct processor capability bits.
> >>>
> >>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >>> Signed-off-by: Michal Wilczynski <michal.wilczynski@...el.com>
> >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> >>> ---
> >>>  arch/x86/include/asm/acpi.h   |  3 +++
> >>>  drivers/acpi/acpi_processor.c | 43 ++++++++++++++++++++++++++++++++++-
> >>>  include/acpi/pdc_intel.h      |  1 +
> >>>  3 files changed, 46 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> >>> index 6a498d1781e7..6c25ce2dad18 100644
> >>> --- a/arch/x86/include/asm/acpi.h
> >>> +++ b/arch/x86/include/asm/acpi.h
> >>> @@ -112,6 +112,9 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
> >>>         if (cpu_has(c, X86_FEATURE_ACPI))
> >>>                 *cap |= ACPI_PDC_T_FFH;
> >>>
> >>> +       if (cpu_has(c, X86_FEATURE_HWP))
> >>> +               *cap |= ACPI_PDC_COLLAB_PROC_PERF;
> >>> +
> >>>         /*
> >>>          * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
> >>>          */
> >>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >>> index 8c5d0295a042..0de0b05b6f53 100644
> >>> --- a/drivers/acpi/acpi_processor.c
> >>> +++ b/drivers/acpi/acpi_processor.c
> >>> @@ -591,13 +591,54 @@ void __init processor_dmi_check(void)
> >>>         dmi_check_system(processor_idle_dmi_table);
> >>>  }
> >>>
> >>> +/* vendor specific UUID indicating an Intel platform */
> >>> +static u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
> >>>  static bool acpi_hwp_native_thermal_lvt_set;
> >>> +static acpi_status __init acpi_processor_osc(acpi_handle handle, u32 lvl,
> >>> +                                            void *context, void **rv)
> >>> +{
> >>> +       u32 capbuf[2] = {};
> >>> +       acpi_status status;
> >>> +       struct acpi_osc_context osc_context = {
> >>> +               .uuid_str = sb_uuid_str,
> >>> +               .rev = 1,
> >>> +               .cap.length = 8,
> >>> +               .cap.pointer = capbuf,
> >>> +       };
> >>> +
> >>> +       if (processor_physically_present(handle) == false)
> >> if (!processor_physically_present(handle))
> > Sure,
> >
> >>> +               return AE_OK;
> >>> +
> >>> +       arch_acpi_set_proc_cap_bits(&capbuf[OSC_SUPPORT_DWORD]);
> >>> +
> >>> +       if (boot_option_idle_override == IDLE_NOMWAIT)
> >>> +               capbuf[OSC_SUPPORT_DWORD] &=
> >>> +                       ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> >>> +
> >>> +       status = acpi_run_osc(handle, &osc_context);
> >>> +       if (ACPI_FAILURE(status))
> >>> +               return status;
> >>> +
> >>> +       if (osc_context.ret.pointer && osc_context.ret.length > 1) {
> >>> +               u32 *capbuf_ret = osc_context.ret.pointer;
> >>> +
> >>> +               if (!acpi_hwp_native_thermal_lvt_set &&
> >>> +                   capbuf_ret[1] & ACPI_PDC_COLLAB_PROC_PERF) {
> >> Checking it in capbuf_ret[] if it was not set in capbuf[] is sort of
> >> questionable.
> >> Note that acpi_hwp_native_thermal_lvt_osc() sets it in capbuf[] before
> >> calling acpi_run_osc().
> > We can add condition before checking capbuf_ret i.e
> >
> > if (capbuf[OSC_SUPPORT_DWORD] & ACPI_PDC_COLLAB_PROC_PERF &&
> >     osc_context.ret.pointer && osc_context.ret.length > 1)
> >
> >
> >>> +                       acpi_handle_info(handle,
> >>> +                                        "_OSC native thermal LVT Acked\n");
> >>> +                       acpi_hwp_native_thermal_lvt_set = true;
> >>> +               }
> >>> +       }
> >>> +       kfree(osc_context.ret.pointer);
> >>> +
> >>> +       return AE_OK;
> >>> +}
> >>> +
> >>>  static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
> >>>                                                           u32 lvl,
> >>>                                                           void *context,
> >>>                                                           void **rv)
> >>>  {
> >>> -       u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
> >>>         u32 capbuf[2];
> >>>         struct acpi_osc_context osc_context = {
> >>>                 .uuid_str = sb_uuid_str,
> >>> diff --git a/include/acpi/pdc_intel.h b/include/acpi/pdc_intel.h
> >>> index 967c552d1cd3..9427f639287f 100644
> >>> --- a/include/acpi/pdc_intel.h
> >>> +++ b/include/acpi/pdc_intel.h
> >>> @@ -16,6 +16,7 @@
> >>>  #define ACPI_PDC_C_C1_FFH              (0x0100)
> >>>  #define ACPI_PDC_C_C2C3_FFH            (0x0200)
> >>>  #define ACPI_PDC_SMP_P_HWCOORD         (0x0800)
> >>> +#define ACPI_PDC_COLLAB_PROC_PERF      (0x1000)
> >> I would call this ACPI_OSC_COLLAB_PROC_PERF to avoid confusion.
> >>
> >> It may also be a good idea to introduce ACPI_OSC_ symbols to replace
> >> the existing ACPI_PDC_ ones (with the same values, respectively) and
> >> get rid of the latter later.
> > Sure I can do that, most likely in a separate commit preceeding this one, so
> > it's easier to explain and review,
>
> Actually on a second thought, maybe instead of creating _OSC specifc constants it would
> be better to just generalize constant names ?

Yes, that would work too.

> As they're the same for both methods, they
> are not really method specific and could be called as follows:
>
> ACPI_PROC_CAP_C_C1_FFH
> ACPI_PROC_CAP_C_C2C3_FFH
>
> So instead of using OSC, or PDC, we just use PROC_CAP, which better explain what those bits
> mean at the end, and removes the hassle of removing those PDC specifc constants in some far
> away future.
>
> Please let me know your thoughts,

Yes, you can do that as far as I am concerned.