[PATCH] acpi: Issue _OSC call for native thermal interrupt handling

[Srinivas Pandruvada]

There are several reports of freeze on enabling HWP (Hardware PStates)
feature on Skylake based systems by Intel P states driver. The root
cause is identified as the HWP interrupts causing BIOS code to freeze.
HWP interrupts uses thermal LVT.
Linux natively handles thermal interrupts, but in Skylake based systems
SMM will take control of thermal interrupts. This is a problem for several
reasons:
- It is freezing in BIOS when tries to handle thermal interrupt, which
will require BIOS upgrade
- With SMM handling thermal we loose all the reporting features of
Linux arch/x86/kernel/cpu/mcheck/therm_throt driver
- Some thermal drivers like x86-package-temp driver depends on the thermal
threshold interrupts
- The HWP interrupts are useful for debugging and tuning performance

So we need native handling of thermal interrupts. To inform SMM that
OS will handle thermal interrupts, we need to use _OSC under processor
scope very early in ACPI initialization flow. This needs to be done
before SMM code path looks for _OSC capabilities. The bit 12 of
_OSC in processor scope defines whether OS will handle thermal interrupts.
When bit 12 is set to 1, OS will handle thermal interrupts.
Refer to this document for details on _OSC
http://www.intel.com/content/www/us/en/standards/processor-vendor-
specific-acpi-specification.html

This change introduces a new function acpi_early_processor_set_osc(),
which walks acpi name space and finds acpi processor object and
set capability via _OSC method to take over thermal LVT.

Also this change writes HWP status bits to 0 to clear any HWP status
bits in intel_thermal_interrupt().

Signed-off-by: Srinivas Pandruvada <srinivas....@linux.intel.com>
---
arch/x86/kernel/cpu/mcheck/therm_throt.c | 8 ++++++
drivers/acpi/acpi_processor.c | 42 +++++++++++++++++++++++++++++++-
drivers/acpi/bus.c | 3 +++
drivers/acpi/internal.h | 2 ++
4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 0b445c2..bb331f6 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -79,6 +79,8 @@ static atomic_t therm_throt_en = ATOMIC_INIT(0);

static u32 lvtthmr_init __read_mostly;

+static bool thermal_hwp_interrupt_support;
+
#ifdef CONFIG_SYSFS
#define define_therm_throt_device_one_ro(_name) \
static DEVICE_ATTR(_name, 0444, \
@@ -384,6 +386,9 @@ static void intel_thermal_interrupt(void)
{
__u64 msr_val;

+ if (thermal_hwp_interrupt_support)
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/
@@ -552,6 +557,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
rdmsr(MSR_IA32_MISC_ENABLE, l, h);
wrmsr(MSR_IA32_MISC_ENABLE, l | MSR_IA32_MISC_ENABLE_TM1, h);

+ if (static_cpu_has(X86_FEATURE_HWP))
+ thermal_hwp_interrupt_support = true;
+
/* Unmask the thermal vector: */
l = apic_read(APIC_LVTTHMR);
apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 6979186..5a78279 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -391,7 +391,6 @@ static int acpi_processor_add(struct acpi_device *device,
if (pr->id >= setup_max_cpus && pr->id != 0)
return 0;
#endif
-
BUG_ON(pr->id >= nr_cpu_ids);

/*
@@ -491,6 +490,47 @@ static void acpi_processor_remove(struct acpi_device *device)
}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */

+static bool acpi_hwp_native_thermal_lvt_set;
+static acpi_status acpi_set_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,
+ .rev = 1,
+ .cap.length = 8,
+ .cap.pointer = capbuf,
+ };
+
+ if (acpi_hwp_native_thermal_lvt_set)
+ return AE_OK;
+
+ if (!static_cpu_has(X86_FEATURE_HWP))
+ return AE_OK;
+
+ capbuf[0] = 0x0000;
+ capbuf[1] = 0x1000; /* set bit 12 */
+
+ if (ACPI_SUCCESS(acpi_run_osc(handle, &osc_context))) {
+ acpi_hwp_native_thermal_lvt_set = true;
+ kfree(osc_context.ret.pointer);
+ }
+
+ return AE_OK;
+}
+
+void acpi_early_processor_set_osc(void)
+{
+ acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX,
+ acpi_set_hwp_native_thermal_lvt_osc,
+ NULL, NULL, NULL);
+ acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
+ acpi_set_hwp_native_thermal_lvt_osc, NULL, NULL);
+}
+
/*
* The following ACPI IDs are known to be suitable for representing as
* processor devices.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 891c42d..7e73aea 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1005,6 +1005,9 @@ static int __init acpi_bus_init(void)
goto error1;
}

+ /* Set capability bits for _OSC under processor scope */
+ acpi_early_processor_set_osc();
+
/*
* _OSC method may exist in module level code,
* so it must be run after ACPI_FULL_INITIALIZATION
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 1e6833a..5c787ac 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -138,6 +138,8 @@ void acpi_early_processor_set_pdc(void);
static inline void acpi_early_processor_set_pdc(void) {}
#endif

+void acpi_early_processor_set_osc(void);
+
/* --------------------------------------------------------------------------
Embedded Controller
-------------------------------------------------------------------------- */
--
1.9.1

[Rafael J. Wysocki]

Has this version of the patch been tested on a system where the problem
is reproducible?

It occurs to me that the whole thing is only necessary on x86, so maybe it can
go under a #ifdef?

Or even to arch/x86/acpi/ (boot.c or a new file)?

> +static bool acpi_hwp_native_thermal_lvt_set;
> +static acpi_status acpi_set_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,
> + .rev = 1,
> + .cap.length = 8,
> + .cap.pointer = capbuf,
> + };
> +
> + if (acpi_hwp_native_thermal_lvt_set)
> + return AE_OK;
> +
> + if (!static_cpu_has(X86_FEATURE_HWP))
> + return AE_OK;

This check can be made once in acpi_early_processor_set_osc().

Thanks,
Rafael

[Srinivas Pandruvada]

---
v2:
Unnecessary newline was introduced, removed that in acpi_processor.c


arch/x86/kernel/cpu/mcheck/therm_throt.c | 8 +++++++
drivers/acpi/acpi_processor.c | 41 ++++++++++++++++++++++++++++++++

drivers/acpi/bus.c | 3 +++
drivers/acpi/internal.h | 2 ++
4 files changed, 54 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 2c5aaf8..4599012 100644

--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -79,6 +79,8 @@ static atomic_t therm_throt_en = ATOMIC_INIT(0);

static u32 lvtthmr_init __read_mostly;

+static bool thermal_hwp_interrupt_support;
+
#ifdef CONFIG_SYSFS
#define define_therm_throt_device_one_ro(_name) \
static DEVICE_ATTR(_name, 0444, \

@@ -385,6 +387,9 @@ static void intel_thermal_interrupt(void)

{
__u64 msr_val;

+ if (thermal_hwp_interrupt_support)
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/

@@ -553,6 +558,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c)

rdmsr(MSR_IA32_MISC_ENABLE, l, h);
wrmsr(MSR_IA32_MISC_ENABLE, l | MSR_IA32_MISC_ENABLE_TM1, h);

+ if (static_cpu_has(X86_FEATURE_HWP))
+ thermal_hwp_interrupt_support = true;
+
/* Unmask the thermal vector: */
l = apic_read(APIC_LVTTHMR);
apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c

index 6979186..d88c463 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -491,6 +491,47 @@ static void acpi_processor_remove(struct acpi_device *device)
}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */

+static bool acpi_hwp_native_thermal_lvt_set;
+static acpi_status acpi_set_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,
+ .rev = 1,
+ .cap.length = 8,
+ .cap.pointer = capbuf,
+ };
+
+ if (acpi_hwp_native_thermal_lvt_set)
+ return AE_OK;
+
+ if (!static_cpu_has(X86_FEATURE_HWP))
+ return AE_OK;

--
2.5.0

[Srinivas Pandruvada]

1.
Yes. I tested on  Yoga 260.
2.
Also in
https://bugzilla.kernel.org/show_bug.cgi?id=110941 
One user confirmed it.
3.
One user in Debian forum
http://forums.debian.net/viewtopic.php?f=7&t=127415

I hope more users will confirm soon as this was reported on many distro
forums also.

I sent this for review and make available for more users to test.

OK. I will submit a new revision.

>
> Or even to arch/x86/acpi/ (boot.c or a new file)?
>
> > +static bool acpi_hwp_native_thermal_lvt_set;
> > +static acpi_status acpi_set_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,
> > + .rev = 1,
> > + .cap.length = 8,
> > + .cap.pointer = capbuf,
> > + };
> > +
> > + if (acpi_hwp_native_thermal_lvt_set)
> > + return AE_OK;
> > +
> > + if (!static_cpu_has(X86_FEATURE_HWP))
> > + return AE_OK;
>
> This check can be made once in acpi_early_processor_set_osc().
>

OK.

I will wait for other comments before submitting another version.

Thanks,
Srinivas

> Thanks,
> Rafael
>

[Srinivas Pandruvada]

---
v3:
- Added CONFIG_X86 around static_cpu_has() to fix compile error on
ARCH=ia64, reported by kbuild test robot
- return AE_CTRL_TERMINATE to terminate acpi name walk space, when _OSC
is set already once.

v2:
Unnecessary newline was introduced, removed that in acpi_processor.c

arch/x86/kernel/cpu/mcheck/therm_throt.c | 8 ++++++

drivers/acpi/acpi_processor.c | 47 ++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 ++
drivers/acpi/internal.h | 2 ++
4 files changed, 60 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 2c5aaf8..4599012 100644

--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -79,6 +79,8 @@ static atomic_t therm_throt_en = ATOMIC_INIT(0);

static u32 lvtthmr_init __read_mostly;

+static bool thermal_hwp_interrupt_support;
+
#ifdef CONFIG_SYSFS
#define define_therm_throt_device_one_ro(_name) \
static DEVICE_ATTR(_name, 0444, \

@@ -385,6 +387,9 @@ static void intel_thermal_interrupt(void)

{
__u64 msr_val;

+ if (thermal_hwp_interrupt_support)
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/

@@ -553,6 +558,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c)

rdmsr(MSR_IA32_MISC_ENABLE, l, h);
wrmsr(MSR_IA32_MISC_ENABLE, l | MSR_IA32_MISC_ENABLE_TM1, h);

+ if (static_cpu_has(X86_FEATURE_HWP))
+ thermal_hwp_interrupt_support = true;
+
/* Unmask the thermal vector: */
l = apic_read(APIC_LVTTHMR);
apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c

index 6979186..55ad24d 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -491,6 +491,53 @@ static void acpi_processor_remove(struct acpi_device *device)
}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */

+#ifdef CONFIG_X86

+static bool acpi_hwp_native_thermal_lvt_set;
+static acpi_status acpi_set_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,
+ .rev = 1,
+ .cap.length = 8,
+ .cap.pointer = capbuf,
+ };
+
+ if (acpi_hwp_native_thermal_lvt_set)

+ return AE_CTRL_TERMINATE;

+
+ capbuf[0] = 0x0000;
+ capbuf[1] = 0x1000; /* set bit 12 */
+
+ if (ACPI_SUCCESS(acpi_run_osc(handle, &osc_context))) {
+ acpi_hwp_native_thermal_lvt_set = true;
+ kfree(osc_context.ret.pointer);
+ }
+
+ return AE_OK;
+}
+
+void acpi_early_processor_set_osc(void)
+{

+ if (static_cpu_has(X86_FEATURE_HWP)) {

+ acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX,
+ acpi_set_hwp_native_thermal_lvt_osc,
+ NULL, NULL, NULL);
+ acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
+ acpi_set_hwp_native_thermal_lvt_osc,

+ NULL, NULL);
+ }
+}
+#else
+
+void acpi_early_processor_set_osc(void) {}
+
+#endif

[Borislav Petkov]

Why the intermittent variable and not using static_cpu_has() directly?

This one can be boot_cpu_has().

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

[Srinivas Pandruvada]

If that's what you prefer, I will do that.

I will give a try.

Thanks,
Srinivas

[Srinivas Pandruvada]

v4:
Suggestion by Boris for removing use of intermediate variable for
clearing HWP status and using boot_cpu_has instead of static_cpu_has

v3:
- Added CONFIG_X86 around static_cpu_has() to fix compile error on
ARCH=ia64, reported by kbuild test robot
- return AE_CTRL_TERMINATE to terminate acpi name walk space, when _OSC
is set already once.
v2:
Unnecessary newline was introduced, removed that in acpi_processor.c

arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 ++

drivers/acpi/acpi_processor.c | 47 ++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 ++
drivers/acpi/internal.h | 2 ++

4 files changed, 55 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 2c5aaf8..0553858 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -385,6 +385,9 @@ static void intel_thermal_interrupt(void)
{
__u64 msr_val;

+ if (static_cpu_has(X86_FEATURE_HWP))

+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 6979186..18da84f 100644

+ if (boot_cpu_has(X86_FEATURE_HWP)) {

1.9.1

[Linda Knippers]

On 3/17/2016 2:24 PM, Srinivas Pandruvada wrote:
> There are several reports of freeze on enabling HWP (Hardware PStates)
> feature on Skylake based systems by Intel P states driver. The root
> cause is identified as the HWP interrupts causing BIOS code to freeze.
> HWP interrupts uses thermal LVT.
> Linux natively handles thermal interrupts, but in Skylake based systems
> SMM will take control of thermal interrupts. This is a problem for several
> reasons:
> - It is freezing in BIOS when tries to handle thermal interrupt, which
> will require BIOS upgrade
> - With SMM handling thermal we loose all the reporting features of
> Linux arch/x86/kernel/cpu/mcheck/therm_throt driver
> - Some thermal drivers like x86-package-temp driver depends on the thermal
> threshold interrupts
> - The HWP interrupts are useful for debugging and tuning performance
>
> So we need native handling of thermal interrupts.

Is this working around a firmware bug?

> To inform SMM that
> OS will handle thermal interrupts, we need to use _OSC under processor
> scope very early in ACPI initialization flow.

Does _OSC say that OS "will" handle thermal interrupts or that it "can"
handle thermal interrupts? Couldn't a platform decide it wants to handle
them, regardless of what the OS may be capable of?

> This needs to be done
> before SMM code path looks for _OSC capabilities. The bit 12 of
> _OSC in processor scope defines whether OS will handle thermal interrupts.
> When bit 12 is set to 1, OS will handle thermal interrupts.
> Refer to this document for details on _OSC
> http://www.intel.com/content/www/us/en/standards/processor-vendor-
> specific-acpi-specification.html

Where is bit 12 documented?

> This change introduces a new function acpi_early_processor_set_osc(),
> which walks acpi name space and finds acpi processor object and
> set capability via _OSC method to take over thermal LVT.

Does this change just affect Skylake platforms or all platforms?

-- ljk

[Srinivas Pandruvada]

Yes. But Linux always had capability to handle thermal interrupts
natively, so we should continue to have this in OS control, when
possible.

>
> >
> > To inform SMM that
> > OS will handle thermal interrupts, we need to use _OSC under
> > processor
> > scope very early in ACPI initialization flow. 
> Does _OSC say that OS "will" handle thermal interrupts or that it
> "can"
> handle thermal interrupts?

Can handle.

>   Couldn't a platform decide it wants to handle
> them, regardless of what the OS may be capable of?
>

Yes. In that case platform can change the delivery mode bits to SMM in
LVT.

> >
> > This needs to be done
> > before SMM code path looks for _OSC capabilities. The bit 12 of
> > _OSC in processor scope defines whether OS will handle thermal
> > interrupts.
> > When bit 12 is set to 1, OS will handle thermal interrupts.
> > Refer to this document for details on _OSC
> > http://www.intel.com/content/www/us/en/standards/processor-vendor-
> > specific-acpi-specification.html
> Where is bit 12 documented?
>

In the above document.

> >
> > This change introduces a new function
> > acpi_early_processor_set_osc(),
> > which walks acpi name space and finds acpi processor object and
> > set capability via _OSC method to take over thermal LVT.
> Does this change just affect Skylake platforms or all platforms?

Any platform which has Intel ® Speed Shift Technology (aka HWP) feature
present and enabled.

Thanks,
Srinivas 

[Linda Knippers]

And would still exhibit the BIOS bug?

>>>
>>> This needs to be done
>>> before SMM code path looks for _OSC capabilities. The bit 12 of
>>> _OSC in processor scope defines whether OS will handle thermal
>>> interrupts.
>>> When bit 12 is set to 1, OS will handle thermal interrupts.
>>> Refer to this document for details on _OSC
>>> http://www.intel.com/content/www/us/en/standards/processor-vendor-
>>> specific-acpi-specification.html
>> Where is bit 12 documented?
>>
> In the above document.

When I look at that document, I see bit 12 described as
"If set, OSPM supports native interrupt handling for Collaborative Processor
Performance Control notifications." Is that the same thing or am
I looking at the wrong table?

-- ljk

[Srinivas Pandruvada]

Yes. But eventually BIOS bug will be fixed.

> >
> > >
> > > >
> > > >
> > > > This needs to be done
> > > > before SMM code path looks for _OSC capabilities. The bit 12 of
> > > > _OSC in processor scope defines whether OS will handle thermal
> > > > interrupts.
> > > > When bit 12 is set to 1, OS will handle thermal interrupts.
> > > > Refer to this document for details on _OSC
> > > > http://www.intel.com/content/www/us/en/standards/processor-vend
> > > > or-
> > > > specific-acpi-specification.html
> > > Where is bit 12 documented?
> > >
> > In the above document.
> When I look at that document, I see bit 12 described as
> "If set, OSPM supports native interrupt handling for Collaborative
> Processor
> Performance Control notifications."  Is that the same thing or am
> I looking at the wrong table?

Yes. If you look at section 14.4 in Intel SDM, you will see that 
"HWP is an implementation of the ACPI-defined Collaborative Processor
Performance Control (CPPC)". Section 14.4.5 also specifies that HWP
uses IA32_THERM_STATUS to communicate if there are notifications, which
is notified via thermal interrupt.

You asked above if platform can handle these notification in SMM only.
If you do then the notification will arrive as ACPI notifications. We
don't have support for such notifications in Linux yet.

Thanks,
Srinivas

>
> -- ljk
>
> >
> >
> > >
> > > >
> > > >
> > > > This change introduces a new function
> > > > acpi_early_processor_set_osc(),
> > > > which walks acpi name space and finds acpi processor object and
> > > > set capability via _OSC method to take over thermal LVT.
> > > Does this change just affect Skylake platforms or all platforms?
> > Any platform which has Intel ® Speed Shift Technology (aka HWP)
> > feature
> > present and enabled.
> >
> > Thanks,
> > Srinivas 
> >
> > >
> > >
> > > -- ljk
> > > >
> > > >
> > > >
> > > > Also this change writes HWP status bits to 0 to clear any HWP
> > > > status
> > > > bits in intel_thermal_interrupt().
> > > >

> > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.i

[Linda Knippers]

On 3/17/2016 5:12 PM, Srinivas Pandruvada wrote:
<snip>

>>>>> This needs to be done
>>>>> before SMM code path looks for _OSC capabilities. The bit 12 of
>>>>> _OSC in processor scope defines whether OS will handle thermal
>>>>> interrupts.
>>>>> When bit 12 is set to 1, OS will handle thermal interrupts.
>>>>> Refer to this document for details on _OSC
>>>>> http://www.intel.com/content/www/us/en/standards/processor-vend
>>>>> or-
>>>>> specific-acpi-specification.html
>>>> Where is bit 12 documented?
>>>>
>>> In the above document.
>> When I look at that document, I see bit 12 described as
>> "If set, OSPM supports native interrupt handling for Collaborative
>> Processor
>> Performance Control notifications." Is that the same thing or am
>> I looking at the wrong table?
> Yes. If you look at section 14.4 in Intel SDM, you will see that
> "HWP is an implementation of the ACPI-defined Collaborative Processor
> Performance Control (CPPC)". Section 14.4.5 also specifies that HWP
> uses IA32_THERM_STATUS to communicate if there are notifications, which
> is notified via thermal interrupt.

Ok, thanks. That wasn't clear from the commit message. It
sounded like bit 12 directly indicated that the OS will handle
thermal interrupts but it's a bit more indirect than that.

> You asked above if platform can handle these notification in SMM only.
> If you do then the notification will arrive as ACPI notifications. We
> don't have support for such notifications in Linux yet.

What I meant to ask was if the platform can disregard the _OSC information
and handle thermal events on it's own, without OS involvement.
For example, servers typically don't want to rely on the OS to manage
thermal issues.

<snip>

>>>>> This change introduces a new function
>>>>> acpi_early_processor_set_osc(),
>>>>> which walks acpi name space and finds acpi processor object and
>>>>> set capability via _OSC method to take over thermal LVT.
>>>> Does this change just affect Skylake platforms or all platforms?
>>> Any platform which has Intel ® Speed Shift Technology (aka HWP)
>>> feature present and enabled.

Could this be an unexpected change in behavior for platforms
with HWP that don't have this bug, assuming they would look at
the _OSC CPPP bit? That's actually my main concern here.

-- ljk

[Rafael J. Wysocki]

Do you have any specific platforms in mind or just in general?

Thanks,
Rafael

[Linda Knippers]

On 3/17/2016 8:17 PM, Rafael J. Wysocki wrote:
<snip>

>>>>>>> This change introduces a new function
>>>>>>> acpi_early_processor_set_osc(),
>>>>>>> which walks acpi name space and finds acpi processor object and
>>>>>>> set capability via _OSC method to take over thermal LVT.
>>>>>> Does this change just affect Skylake platforms or all platforms?
>>>>> Any platform which has Intel ® Speed Shift Technology (aka HWP)
>>>>> feature present and enabled.
>>
>> Could this be an unexpected change in behavior for platforms
>> with HWP that don't have this bug, assuming they would look at
>> the _OSC CPPP bit? That's actually my main concern here.
>
> Do you have any specific platforms in mind or just in general?

It's just general right now but as more HWP server platforms come out,
it may become specific.

-- ljk
>
> Thanks,
> Rafael
>

[Srinivas Pandruvada]

There are several reports of freeze on enabling HWP (Hardware PStates)
feature on Skylake based systems by Intel P states driver. The root
cause is identified as the HWP interrupts causing BIOS code to freeze.
HWP interrupts uses thermal LVT.
Linux natively handles thermal interrupts, but in Skylake based systems
SMM will take control of thermal interrupts. This is a problem for several
reasons:
- It is freezing in BIOS when tries to handle thermal interrupt, which
will require BIOS upgrade
- With SMM handling thermal we loose all the reporting features of
Linux arch/x86/kernel/cpu/mcheck/therm_throt driver
- Some thermal drivers like x86-package-temp driver depends on the thermal
threshold interrupts
- The HWP interrupts are useful for debugging and tuning performance

So we need native handling of thermal interrupts. This requires some
way to inform SMM that OS can handle thermal interrupts. This can be
done by using _OSC/_PDC under processor scope very early in ACPI
initialization flow.
The bit 12 of _OSC/_PDC in processor scope defines whether OS supports
handling of native interrupts for Collaborative Processor Performance
Control (CPPC) notifications. Since HWP is a implementation of CPPC,
setting this bit is equivalent to inform SMM that OS is capable of
handling thermal interrupts.
Refer to this document for details on _OSC/_PDC
http://www.intel.com/content/www/us/en/standards/processor-vendor-
specific-acpi-specification.html

This change introduces a new function acpi_early_processor_set_osc(),
which walks acpi name space and finds acpi processor object and

set capability via _OSC method.

Also this change writes HWP status bits to 0 to clear any HWP status
bits in intel_thermal_interrupt().

Signed-off-by: Srinivas Pandruvada <srinivas....@linux.intel.com>
---
v5:
No code change. Changed commit message to explain HWP is a implementation
of CPPC.

v4:
Suggestion by Boris for removing use of intermediate variable for
clearing HWP status and using boot_cpu_has instead of static_cpu_has

v3:
- Added CONFIG_X86 around static_cpu_has() to fix compile error on
ARCH=ia64, reported by kbuild test robot
- return AE_CTRL_TERMINATE to terminate acpi name walk space, when _OSC
is set already once.
v2:
Unnecessary newline was introduced, removed that in acpi_processor.c

arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 ++
drivers/acpi/acpi_processor.c | 47 ++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 ++
drivers/acpi/internal.h | 2 ++
4 files changed, 55 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c

index 0b445c2..ac780ca 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -384,6 +384,9 @@ static void intel_thermal_interrupt(void)

{
__u64 msr_val;

+ if (static_cpu_has(X86_FEATURE_HWP))
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c

index b5e54f2..52934d9 100644

index 0e85678..1f65eb5 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1019,6 +1019,9 @@ static int __init acpi_bus_init(void)

goto error1;
}

+ /* Set capability bits for _OSC under processor scope */
+ acpi_early_processor_set_osc();
+
/*
* _OSC method may exist in module level code,
* so it must be run after ACPI_FULL_INITIALIZATION
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h

index a37508e..0aaeef1 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -145,6 +145,8 @@ void acpi_early_processor_set_pdc(void);

static inline void acpi_early_processor_set_pdc(void) {}
#endif

+void acpi_early_processor_set_osc(void);
+

/* --------------------------------------------------------------------------
Embedded Controller
-------------------------------------------------------------------------- */

--
1.9.1

[Rafael J. Wysocki]

That can be __init, can't it?

And that can be __init too?

> +{
> + if (boot_cpu_has(X86_FEATURE_HWP)) {
> + acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
> + ACPI_UINT32_MAX,
> + acpi_set_hwp_native_thermal_lvt_osc,
> + NULL, NULL, NULL);
> + acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
> + acpi_set_hwp_native_thermal_lvt_osc,
> + NULL, NULL);
> + }
> +}
> +#else
> +
> +void acpi_early_processor_set_osc(void) {}

It's better to define it in the header as a static inline IMO.

#ifdef CONFIG_X86

> +void acpi_early_processor_set_osc(void);

#else
static inline void acpi_early_processor_set_osc(void) {}
#endif

[Srinivas Pandruvada]

v6:
Added __init for two functions and moved dummy function to internal.h
Shortened the name of the function.

v5:
No code change. Changed commit message to explain HWP is a implementation
of CPPC.

v4:
Suggestion by Boris for removing use of intermediate variable for
clearing HWP status and using boot_cpu_has instead of static_cpu_has

v3:
- Added CONFIG_X86 around static_cpu_has() to fix compile error on
ARCH=ia64, reported by kbuild test robot
- return AE_CTRL_TERMINATE to terminate acpi name walk space, when _OSC
is set already once.
v2:
Unnecessary newline was introduced, removed that in acpi_processor.c

arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 +++
drivers/acpi/acpi_processor.c | 44 ++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 +++
drivers/acpi/internal.h | 6 +++++
4 files changed, 56 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 2c5aaf8..0553858 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -385,6 +385,9 @@ static void intel_thermal_interrupt(void)

{
__u64 msr_val;

+ if (static_cpu_has(X86_FEATURE_HWP))
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c

index 6979186..ba50f46 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -491,6 +491,50 @@ static void acpi_processor_remove(struct acpi_device *device)

}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */

+#ifdef CONFIG_X86
+static bool acpi_hwp_native_thermal_lvt_set;

+static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
+ u32 lvl,
+ void *context,

+void __init acpi_early_processor_set_osc(void)

+{
+ if (boot_cpu_has(X86_FEATURE_HWP)) {
+ acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX,

+ acpi_hwp_native_thermal_lvt_osc,

+ NULL, NULL, NULL);
+ acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,

+ acpi_hwp_native_thermal_lvt_osc,

+ NULL, NULL);
+ }
+}

+#endif
+
/*
* The following ACPI IDs are known to be suitable for representing as
* processor devices.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c

index 891c42d..7e73aea 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1005,6 +1005,9 @@ static int __init acpi_bus_init(void)

goto error1;
}

+ /* Set capability bits for _OSC under processor scope */
+ acpi_early_processor_set_osc();
+
/*
* _OSC method may exist in module level code,
* so it must be run after ACPI_FULL_INITIALIZATION
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h

index 1e6833a..9f34051b 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -138,6 +138,12 @@ void acpi_early_processor_set_pdc(void);

static inline void acpi_early_processor_set_pdc(void) {}
#endif

+#ifdef CONFIG_X86
+void acpi_early_processor_set_osc(void);
+#else
+static inline void acpi_early_processor_set_osc(void) {}
+#endif

+
/* --------------------------------------------------------------------------
Embedded Controller
-------------------------------------------------------------------------- */
--

2.5.0

[Linda Knippers]

I raised a general concern on a previous patch so I found a 1P server
with Skylake and HWP to try. This doesn't qualify as a tested-by
since all I did was apply the patch and boot the server but hey, it booted.

I do have a question below...

There are other boot messages that indicate when something is happening
with _OSC. Should there be one for this? Or is there some other obvious
way one can know that this was set?

[Srinivas Pandruvada]

On Wed, 2016-03-23 at 12:43 -0400, Linda Knippers wrote:
> I raised a general concern on a previous patch so I found a 1P server
> with Skylake and HWP to try.  This doesn't qualify as a tested-by
> since all I did was apply the patch and boot the server but hey, it
> booted.

Thanks.

>
> I do have a question below...
>

[...]

> + if (ACPI_SUCCESS(acpi_run_osc(handle, &osc_context))) {
> > + acpi_hwp_native_thermal_lvt_set = true;
> > + kfree(osc_context.ret.pointer);
>
> There are other boot messages that indicate when something is
> happening
> with _OSC.  Should there be one for this?  Or is there some other
> obvious
> way one can know that this was set?
>

I am following model of acpi_bus_osc_support, which issues _OSC for
global platform scope, where nothing is getting printed. If it is
useful, I don't mind adding a print.

Rafael,
What do you think?

Thanks,
Srinivas

[Rafael J. Wysocki]

Printing a message after acpi_run_osc() has been called successfully
shouldn't hurt. Maybe using acpi_handle_info()?

Thanks,
Rafael

[Srinivas Pandruvada]

v7:
Checking platform ack and setting status for successful ack and printing
information message.

v6:
Added __init for two functions and moved dummy function to internal.h
Shortened the name of the function.

v5:
No code change. Changed commit message to explain HWP is a implementation
of CPPC.

v4:
Suggestion by Boris for removing use of intermediate variable for
clearing HWP status and using boot_cpu_has instead of static_cpu_has

v3:
- Added CONFIG_X86 around static_cpu_has() to fix compile error on
ARCH=ia64, reported by kbuild test robot
- return AE_CTRL_TERMINATE to terminate acpi name walk space, when _OSC
is set already once.
v2:
Unnecessary newline was introduced, removed that in acpi_processor.c

arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 ++

drivers/acpi/acpi_processor.c | 52 ++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 3 ++
drivers/acpi/internal.h | 6 ++++
4 files changed, 64 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 0b445c2..ac780ca 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -384,6 +384,9 @@ static void intel_thermal_interrupt(void)

{
__u64 msr_val;

+ if (static_cpu_has(X86_FEATURE_HWP))
+ wrmsrl_safe(MSR_HWP_STATUS, 0);
+
rdmsrl(MSR_IA32_THERM_STATUS, msr_val);

/* Check for violation of core thermal thresholds*/
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c

index b5e54f2..4239b80 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -491,6 +491,58 @@ static void acpi_processor_remove(struct acpi_device *device)

}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */

+#ifdef CONFIG_X86
+static bool acpi_hwp_native_thermal_lvt_set;
+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,
+ .rev = 1,
+ .cap.length = 8,
+ .cap.pointer = capbuf,
+ };
+
+ if (acpi_hwp_native_thermal_lvt_set)
+ return AE_CTRL_TERMINATE;
+
+ capbuf[0] = 0x0000;
+ capbuf[1] = 0x1000; /* set bit 12 */
+

+ if (ACPI_SUCCESS(acpi_run_osc(handle, &osc_context))) {
+ if (osc_context.ret.pointer && osc_context.ret.length > 1) {
+ u32 *capbuf_ret = osc_context.ret.pointer;
+
+ if (capbuf_ret[1] & 0x1000) {
+ 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;
+}
+
+void __init acpi_early_processor_set_osc(void)
+{
+ if (boot_cpu_has(X86_FEATURE_HWP)) {
+ acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX,
+ acpi_hwp_native_thermal_lvt_osc,
+ NULL, NULL, NULL);
+ acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
+ acpi_hwp_native_thermal_lvt_osc,
+ NULL, NULL);
+ }
+}
+#endif
+
/*
* The following ACPI IDs are known to be suitable for representing as
* processor devices.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c

index 0e85678..1f65eb5 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1019,6 +1019,9 @@ static int __init acpi_bus_init(void)

goto error1;
}

+ /* Set capability bits for _OSC under processor scope */
+ acpi_early_processor_set_osc();
+
/*
* _OSC method may exist in module level code,
* so it must be run after ACPI_FULL_INITIALIZATION
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h

index a37508e..9ed878d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -145,6 +145,12 @@ void acpi_early_processor_set_pdc(void);

static inline void acpi_early_processor_set_pdc(void) {}
#endif

+#ifdef CONFIG_X86
+void acpi_early_processor_set_osc(void);
+#else
+static inline void acpi_early_processor_set_osc(void) {}
+#endif
+
/* --------------------------------------------------------------------------
Embedded Controller
-------------------------------------------------------------------------- */

--
1.9.1

[Rafael J. Wysocki]

I've queued it up, but I have modified the subject and the changelog
and renamed the new function.

Please have a look at
http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=42341f87ba1bee4c5be95038c24abb69cbcf361a
and let me know if it makes sense to you.

Thanks,
Rafael

[Srinivas Pandruvada]

Looks fine to me.

Thanks,
Srinivas

>
> Thanks,
> Rafael