On 6/8/20 1:15 PM, Sean Christopherson wrote:
> On Mon, Jun 08, 2020 at 08:21:14AM -0400, Prarit Bhargava wrote:
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index 166d7c355896..e02ec81fe1eb 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -1074,10 +1074,17 @@ static void split_lock_init(void)
>> split_lock_verify_msr(sld_state != sld_off);
>> }
>>
>> -static void split_lock_warn(unsigned long ip)
>> +static bool split_lock_warn(unsigned long ip, int fatal_no_warn)
>> {
>> - pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address:
>> 0x%lx\n",
>> - current->comm, current->pid, ip);
>> + if (fatal_no_warn)
>> + return false;
>
> This misses the point Xiaoyao was making. If EFLAGS.AC=1 then the #AC is a
> legacy alignment check fault and should not be treated as a split-lock #AC.
> The basic premise of the patch makes sense, but the end result is confusing
> because incorporating "fatal" and the EFLAGS.AC state into split_lock_warn()
> bastardizes both the "split_lock" and "warn" aspects of the function.
>
> E.g. something like this yields the same net effect, it's just organized
> differently. If so desired, the "bogus" message could be dropped via
> Xiaoyao's prep patch[*] so that this change would only affect the sld_fatal
> messages.
>
> [*] https://lkml.kernel.org/r/20200509110542.8159-3-xiaoyao...@intel.com
>
>
Sean, I will just take your patch to make things easy. I will add you as a
Signed-off-by.
/me is testing the patch right now
P.
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 23fd5f319908..1aad0b8e394c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -1071,11 +1071,14 @@ static void split_lock_init(void)
> split_lock_verify_msr(sld_state != sld_off);
> }
>
> -static void split_lock_warn(unsigned long ip)
> +static bool handle_split_lock(unsigned long ip)
> {
> pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address:
> 0x%lx\n",
> current->comm, current->pid, ip);
>
> + if (sld_state != sld_warn)
> + return false;
> +
> /*
> * Disable the split lock detection for this task so it can make
> * progress and set TIF_SLD so the detection is re-enabled via
> @@ -1083,18 +1086,13 @@ static void split_lock_warn(unsigned long ip)
> */
> sld_update_msr(false);
> set_tsk_thread_flag(current, TIF_SLD);
> + return true;
> }
>
> bool handle_guest_split_lock(unsigned long ip)
> {
> - if (sld_state == sld_warn) {
> - split_lock_warn(ip);
> + if (handle_split_lock(ip))
> return true;
> - }
> -
> - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n",
> - current->comm, current->pid,
> - sld_state == sld_fatal ? "fatal" : "bogus", ip);
>
> current->thread.error_code = 0;
> current->thread.trap_nr = X86_TRAP_AC;
> @@ -1105,10 +1103,10 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock);
>
> bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> {
> - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
> + if (regs->flags & X86_EFLAGS_AC)
> return false;
> - split_lock_warn(regs->ip);
> - return true;
> +
> + return handle_split_lock(regs->ip);
> }
>
> /*
>
>
>> +
>> + pr_warn_ratelimited("#AC: %s/%d %ssplit_lock trap at address: 0x%lx\n",
>> + current->comm, current->pid,
>> + sld_state == sld_fatal ? "fatal " : "", ip);
>> +
>> + if (sld_state == sld_fatal)
>> + return false;
>>
>> /*
>> * Disable the split lock detection for this task so it can make
>> @@ -1086,18 +1093,13 @@ static void split_lock_warn(unsigned long ip)
>> */
>> sld_update_msr(false);
>> set_tsk_thread_flag(current, TIF_SLD);
>> + return true;
>> }
>>
>> bool handle_guest_split_lock(unsigned long ip)
>> {
>> - if (sld_state == sld_warn) {
>> - split_lock_warn(ip);
>> + if (split_lock_warn(ip, 0))
>> return true;
>> - }
>> -
>> - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n",
>> - current->comm, current->pid,
>> - sld_state == sld_fatal ? "fatal" : "bogus", ip);
>>
>> current->thread.error_code = 0;
>> current->thread.trap_nr = X86_TRAP_AC;
>> @@ -1108,10 +1110,7 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock);
>>
>> bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>> {
>> - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
>> - return false;
>> - split_lock_warn(regs->ip);
>> - return true;
>> + return split_lock_warn(regs->ip, regs->flags & X86_EFLAGS_AC);
>> }
>>
>> /*
>> --
>> 2.21.3
>>
>
