Subject: Re: [RFC PATCH 0/6] Unify the Interrupt Mode and setup it as soon as
 possible
To: Thomas Gleixner <tglx@linutronix.de>
References: <1490799333-18242-1-git-send-email-douly.fnst@cn.fujitsu.com>
 <alpine.DEB.2.20.1704060955460.1716@nanos>
CC: <x86@kernel.org>, <linux-kernel@vger.kernel.org>, <mingo@kernel.org>,
        <ebiederm@xmission.com>, <bhe@redhat.com>, <hpa@zytor.com>,
        <izumi.taku@jp.fujitsu.com>
From: Dou Liyang <douly.fnst@cn.fujitsu.com>
Message-ID: <71b049b0-1e4d-a885-91d3-03328cafb65f@cn.fujitsu.com>
Date: Fri, 7 Apr 2017 17:39:04 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.20.1704060955460.1716@nanos>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3119
Lines: 135

Hi Thomas,

At 04/06/2017 04:43 PM, Thomas Gleixner wrote:
> On Wed, 29 Mar 2017, Dou Liyang wrote:
>> The purpose of this patchset is Unifing these setup steps and executing as
>> soon as possible as follows:
>>
>>    start_kernel
>> ---------------+
>> |
>> |
>> |
>> |     init_IRQ
>> +---->---+----+
>> |        |
>> |        |      +--------------------+
>> |        +----> | 4. init_bsp_APIC   |
>> |               +--------------------+
>> v
>>
>> By the way, Also fix a bug about kexec[3].
>>
>>
>> Some doubts, need help:
>>
>> 1. Patchset has influence on IOMMU in enable_IR_x2apic(). Not sure
>> it can be in advance?
>
> That should work.
>

Got it. Thanks very much.

>> 2. Due to
>>
>> Commit 8c3ba8d04924 ("x86, apic: ack all pending irqs when crashed/on kexec")
>>
>>  ..., patchset also needs TSC and uses the "cpu_khz" in setup_local_APIC().
>> And a warning[4] will be triggered when crashed/on kexec. Not sure how to
>> modify?
>
> The local APIC timer initialization cannot be run from init_IRQ().
>

Yes, I think so.

> The problem here is that CPU and TSC frequency calibration depends on the
> PIT/HPET interrupt (irq 0) working for machines which cannot calibrate via
> MSR/CPUID or if fast calibration via PIT fails.
>

Yes, we use the CPU frequency and tsc before we calibrate them.

> So we need to split that initialization into several parts:
>
> 1) Set up the APIC/IOAPIC (including testing whether the timer interrupt
>    works)
>
> 2) Calibrate TSC
>
> 3) Set up the local APIC timer
>

Yes, correct. the patchset has splited the initialization like that.
And I don't change anything in part 2 to reduce the impact.

For the problem above, the reason is:

When do part 1 in dump-capture kernel, need clear ISR for the pending
interrupt from previous kernel, and use CPU frequency and TSC for
calculating the maximum number of cycles.

But, the CPU frequency and TSC is calibrated in part 2 late than part 1.

So, in part 1, the CPU frequency is 0, and calibrate a wrong maximum
loops.

I try to replace the loops calibration with a new way, which has
nothing to do with TSC, such as set a fixed value for max_loops. Is
that OK?

Or, may need do some work in part 1, 2, re-split the initialization.


Here is the code for clearing ISR:

unsigned long long tsc = 0, ntsc;
	long long max_loops = cpu_khz ? cpu_khz : 1000000;

	if (boot_cpu_has(X86_FEATURE_TSC))
		tsc = rdtsc();
......
......

	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 (j = 31; j >= 0; j--) {
				if (value & (1<<j)) {
					ack_APIC_irq();
					acked++;
				}
			}
		}
		if (acked > 256) {
			printk(KERN_ERR "LAPIC pending interrupts after %d EOI\n",
			       acked);
			break;
		}
		if (queued) {
			if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
				ntsc = rdtsc();
				max_loops = (cpu_khz << 10) - (ntsc - tsc);
			} else
				max_loops--;
		}
	} while (queued && max_loops > 0);
	WARN_ON(max_loops <= 0);
... ...


Thanks,
	Liyang.

> Thanks,
>
> 	tglx
>
>
>