linux-kernel - Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp

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: <174164c8-bca5-8c3b-d0f1-ae75a3a8a0d6@cn.fujitsu.com>
Date:   Fri, 7 Apr 2017 15:51:11 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <mingo@...nel.org>, <ebiederm@...ssion.com>, <bhe@...hat.com>,
        <hpa@...or.com>, <izumi.taku@...fujitsu.com>
Subject: Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from
 apic_bsp_setup()

Hi Thomas,

At 04/05/2017 07:56 PM, Thomas Gleixner wrote:
> On Wed, 29 Mar 2017, Dou Liyang wrote:
>> +/* Setup local APIC timer and get the Id*/
>> +static int __init apic_bsp_timer_setup(void)
>
> This does not make sense. The id and the timers have nothing to do with
> each other.

Yes, Indeed. Here is more like the rest work for APIC setup, which
can't be executed earlier, The name is not suitable. I will refactor
it.

>> +{
>> +	int id;
>> +
>> +	if (x2apic_mode)
>> +		id = apic_read(APIC_LDR);
>> +	else
>> +		id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
>> +
>> +	if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
>> +		check_timer();
>
> Why are you moving this to the APIC? check_timer() has absolutely nothing

Because, the jiffies is used in check_timer() for checking timer irq 
works(timer_irq_works()) and can't work at the beginning.

So, keep check_timer() in IOAPIC setup function (setup_IO_APIC()) which
has been moved to init_IRQ() will make the kernel not boot up.

> to do with the apic timer. It's a IOAPIC only issue and required to test
> that the IRQ0 interrupt delivery works through the IOAPIC.
>

Yes, I see, split check_timer() is not a good way, and move the APIC
initialization to the end of init_IRQ() is also not well.

Thanks,
	Liyang

> Thanks,
>
> 	tglx
>
>
>