2021-09-01 18:50:42

by Chenyi Qiang

Subject: [PATCH] x86/bus_lock: Don't assume the init value of DEBUGCTLMSR.BUS_LOCK_DETECT to be zero

It's possible that BIOS/firmware has set DEBUGCTLMSR_BUS_LOCK_DETECT, or
this kernel has been kexec'd from a kernel that enabled bus lock
detection.

Disable bus lock detection explicitly if not wanted.

Signed-off-by: Chenyi Qiang <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
---
arch/x86/kernel/cpu/intel.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 8321c43554a1..38dda04d9342 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1152,22 +1152,23 @@ static void bus_lock_init(void)
{
u64 val;

- /*
- * Warn and fatal are handled by #AC for split lock if #AC for
- * split lock is supported.
- */
- if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) ||
- (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
- (sld_state == sld_warn || sld_state == sld_fatal)) ||
- sld_state == sld_off)
+ if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
return;

- /*
- * Enable #DB for bus lock. All bus locks are handled in #DB except
- * split locks are handled in #AC in the fatal case.
- */
rdmsrl(MSR_IA32_DEBUGCTLMSR, val);
- val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
+
+ if ((boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
+ (sld_state == sld_warn || sld_state == sld_fatal)) ||
+ sld_state == sld_off) {
+ /*
+ * Warn and fatal are handled by #AC for split lock if #AC for
+ * split lock is supported.
+ */
+ val &= ~DEBUGCTLMSR_BUS_LOCK_DETECT;
+ } else {
+ val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
+ }
+
wrmsrl(MSR_IA32_DEBUGCTLMSR, val);
}

--
2.17.1

2021-09-17 10:41:16

by Chenyi Qiang

Subject: Re: [PATCH] x86/bus_lock: Don't assume the init value of DEBUGCTLMSR.BUS_LOCK_DETECT to be zero

Kindly ping for this minor change.

Thanks
Chenyi

On 9/1/2021 4:40 PM, Chenyi Qiang wrote:
> It's possible that BIOS/firmware has set DEBUGCTLMSR_BUS_LOCK_DETECT, or
> this kernel has been kexec'd from a kernel that enabled bus lock
> detection.
>
> Disable bus lock detection explicitly if not wanted.
>
> Signed-off-by: Chenyi Qiang <[email protected]>
> Reviewed-by: Tony Luck <[email protected]>
> ---
> arch/x86/kernel/cpu/intel.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 8321c43554a1..38dda04d9342 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -1152,22 +1152,23 @@ static void bus_lock_init(void)
> {
> u64 val;
>
> - /*
> - * Warn and fatal are handled by #AC for split lock if #AC for
> - * split lock is supported.
> - */
> - if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) ||
> - (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
> - (sld_state == sld_warn || sld_state == sld_fatal)) ||
> - sld_state == sld_off)
> + if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
> return;
>
> - /*
> - * Enable #DB for bus lock. All bus locks are handled in #DB except
> - * split locks are handled in #AC in the fatal case.
> - */
> rdmsrl(MSR_IA32_DEBUGCTLMSR, val);
> - val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
> +
> + if ((boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
> + (sld_state == sld_warn || sld_state == sld_fatal)) ||
> + sld_state == sld_off) {
> + /*
> + * Warn and fatal are handled by #AC for split lock if #AC for
> + * split lock is supported.
> + */
> + val &= ~DEBUGCTLMSR_BUS_LOCK_DETECT;
> + } else {
> + val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
> + }
> +
> wrmsrl(MSR_IA32_DEBUGCTLMSR, val);
> }
>
>

2021-10-19 07:37:24

by Chenyi Qiang

Subject: Re: [PATCH] x86/bus_lock: Don't assume the init value of DEBUGCTLMSR.BUS_LOCK_DETECT to be zero

Reminder for this minor fix.

On 9/17/2021 10:52 AM, Chenyi Qiang wrote:
> Kindly ping for this minor change.
>
> Thanks
> Chenyi
>
> On 9/1/2021 4:40 PM, Chenyi Qiang wrote:
>> It's possible that BIOS/firmware has set DEBUGCTLMSR_BUS_LOCK_DETECT, or
>> this kernel has been kexec'd from a kernel that enabled bus lock
>> detection.
>>
>> Disable bus lock detection explicitly if not wanted.
>>
>> Signed-off-by: Chenyi Qiang <[email protected]>
>> Reviewed-by: Tony Luck <[email protected]>
>> ---
>>   arch/x86/kernel/cpu/intel.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index 8321c43554a1..38dda04d9342 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -1152,22 +1152,23 @@ static void bus_lock_init(void)
>>   {
>>       u64 val;
>> -    /*
>> -     * Warn and fatal are handled by #AC for split lock if #AC for
>> -     * split lock is supported.
>> -     */
>> -    if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) ||
>> -        (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
>> -        (sld_state == sld_warn || sld_state == sld_fatal)) ||
>> -        sld_state == sld_off)
>> +    if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
>>           return;
>> -    /*
>> -     * Enable #DB for bus lock. All bus locks are handled in #DB except
>> -     * split locks are handled in #AC in the fatal case.
>> -     */
>>       rdmsrl(MSR_IA32_DEBUGCTLMSR, val);
>> -    val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
>> +
>> +    if ((boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
>> +        (sld_state == sld_warn || sld_state == sld_fatal)) ||
>> +        sld_state == sld_off) {
>> +        /*
>> +         * Warn and fatal are handled by #AC for split lock if #AC for
>> +         * split lock is supported.
>> +         */
>> +        val &= ~DEBUGCTLMSR_BUS_LOCK_DETECT;
>> +    } else {
>> +        val |= DEBUGCTLMSR_BUS_LOCK_DETECT;
>> +    }
>> +
>>       wrmsrl(MSR_IA32_DEBUGCTLMSR, val);
>>   }
>>

2021-10-19 17:07:23

by Sean Christopherson

Subject: Re: [PATCH] x86/bus_lock: Don't assume the init value of DEBUGCTLMSR.BUS_LOCK_DETECT to be zero

On Wed, Sep 01, 2021, Chenyi Qiang wrote:
> It's possible that BIOS/firmware has set DEBUGCTLMSR_BUS_LOCK_DETECT, or
> this kernel has been kexec'd from a kernel that enabled bus lock
> detection.

This feels like the kernel should explicitly zero out the entire MSR somewhere
in the generic boot flow. E.g. something like this somewhere.

#ifndef CONFIG_X86_DEBUGCTLMSR
if (boot_cpu_data.x86 < 6)
return;
#endifa

wrmsrl(MSR_IA32_DEBUGCTLMSR, 0);

2021-10-25 06:57:00

by Chenyi Qiang

Subject: Re: [PATCH] x86/bus_lock: Don't assume the init value of DEBUGCTLMSR.BUS_LOCK_DETECT to be zero

On 10/20/2021 1:05 AM, Sean Christopherson wrote:
> On Wed, Sep 01, 2021, Chenyi Qiang wrote:
>> It's possible that BIOS/firmware has set DEBUGCTLMSR_BUS_LOCK_DETECT, or
>> this kernel has been kexec'd from a kernel that enabled bus lock
>> detection.
>
> This feels like the kernel should explicitly zero out the entire MSR somewhere
> in the generic boot flow. E.g. something like this somewhere.
>

Yes. Meanwhile, I think kernel code prefers to explicitly set/clear the
control bit according to the parameter. Maybe both changes should be
applied.

> #ifndef CONFIG_X86_DEBUGCTLMSR
> if (boot_cpu_data.x86 < 6)
> return;
> #endifa
>
> wrmsrl(MSR_IA32_DEBUGCTLMSR, 0);
>