On Mon, Jul 3, 2023 at 11:52 AM Wilczynski, Michal <michal.wilczynski@xxxxxxxxx> wrote: > > > > On 6/30/2023 11:23 AM, Wilczynski, Michal wrote: > > > > On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote: > >> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal > >> <michal.wilczynski@xxxxxxxxx> wrote: > >>> > >>> On 6/29/2023 3:15 PM, Rafael J. Wysocki wrote: > >>>> On Thu, Jun 29, 2023 at 1:04 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > >>>>> I would just say "Introduce acpi_processor_osc()" in the subject and > >>>>> then explain its role in the changelog. > >>>>> > >>>>> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski > >>>>> <michal.wilczynski@xxxxxxxxx> 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@xxxxxxxxx> > >>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx> > >>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > >>>>>> --- > >>>>>> 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)) > >>>>> > >>>>>> + 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(). > >>>> So you moved setting it to arch_acpi_set_proc_cap_bits(), but then it > >>>> should also be checked by the arch code. That is, add an arch > >>>> function to check if a given bit is set in the returned capabilities > >>>> buffer (passed as an argument). > >>> Hmm, maybe that's true, but the only reason we check that is so we can print > >>> a debug message > >> No, it is not the only reason. The more important part is to set > >> acpi_hwp_native_thermal_lvt_set if it is still unset at that point. > > Yeah, that too. Okay I'll modify the code > > > >>> - that's pretty much a leftover after a workaround. Introducing > >>> more arch code to accommodate this seemed wasteful, since in my understanding > >>> all workarounds are meant to be removed at some point, even if it takes a long time > >>> to do so. > >> Not really, until the systems needing them are in use. > > Yeah I suspect it might take a very long time, and I guess it's very hard to definitely > > say that some piece of hardware is not used by **anyone** > > > >>>> Also it can be argued that ACPI_PDC_C_C2C3_FFH and ACPI_PDC_C_C1_FFH > >>>> should be set by the arch code too. > >>> That makes sense, but technically is also a workaround, since we're basically > >>> checking for some specific DMI's and then we disable mwait for them. > >> But boot_option_idle_override is set by the arch code, isn't it? > >> > >> So the arch code may as well do the quirk in accordance with it. > > I think so, I'll modify the code to move setting those bits to the arch part > > I looked into that, and I'm still not sure whether setting those constants in arch > specific code is a good idea. Basically OSC and PDC are supported on two architectures > ia64 and x86, so that would introduce unnecessary code duplication, as this mechanic > is present regardless of an architecture, and in this particular case boot_option_idle_override > is set by acpi_processor.c function set_no_mwait(). Which is x86-specific AFAICS. > One could argue theoretically that system defined in processor_dmi_table[] is an x86 so there > is no need to add any logic to ia64, Good observation! > but to me this is confusing. Why is it so? > If we have a workaround in the acpi_processor, maybe entire workaround should stay there > instead of dragging innocent arch code into it. And maybe it would be better to move it to arch code as a whole.