linux-kernel - Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function

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: <20180223070854.bjtybuj5qnztumaz@gmail.com>
Date:   Fri, 23 Feb 2018 08:08:54 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Dou Liyang <douly.fnst@...fujitsu.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
        ebiederm@...ssion.com, bhe@...hat.com, andy.shevchenko@...il.com
Subject: Re: [PATCH v2] x86/apic: Move pending intr check code into it's own
 function


* Dou Liyang <douly.fnst@...fujitsu.com> wrote:

> the pending interrupt check code is mixed with the local APIC setup code,
> that looks messy.
> 
> Extract the related code, move it into a new function named
> apic_pending_intr_clear().
> 
> bonus cleanups from Andy Shevchenko's suggestions:
> 
>   - for() -> for_each_set_bit()
>   - printk() -> pr_err()

Please split the cleanups (and the cleanups suggested further below) into a 
separate patch, so that there's a pure 'code movement' patch plus another patch 
that is easy to review.

> +	/*
> +	 * After a crash, we no longer service the interrupts and a pending
> +	 * interrupt from previous kernel might still have ISR bit set.
> +	 *
> +	 * Most probably by now CPU has serviced that pending interrupt and
> +	 * it might not have done the ack_APIC_irq() because it thought,
> +	 * interrupt came from i8259 as ExtInt. LAPIC did not get EOI so it
> +	 * does not clear the ISR bit and cpu thinks it has already serivced
> +	 * the interrupt. Hence a vector might get locked. It was noticed
> +	 * for timer irq (vector 0x31). Issue an extra EOI to clear ISR.
> +	 */
> +	do {
> +		queued = 0;
> +		for (i = APIC_ISR_NR - 1; i >= 0; i--)
> +			queued |= apic_read(APIC_IRR + i*0x10);
> +
> +		for (i = APIC_ISR_NR - 1; i >= 0; i--) {
> +			value = apic_read(APIC_ISR + i*0x10);
> +			for_each_set_bit(j, &value, 32) {
> +				if (j) {
> +					ack_APIC_irq();
> +					acked++;
> +				}
> +			}
> +		}
> +		if (acked > 256) {
> +			pr_err("LAPIC pending interrupts after %d EOI\n",
> +				acked);

Please don't
	break the line of
	printk's.


> +		if (queued) {
> +			if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
> +				ntsc = rdtsc();
> +				max_loops = (cpu_khz << 10) - (ntsc - tsc);
> +			} else
> +				max_loops--;

unbalanced curly braces.

Thanks,

	Ingo