linux-kernel - Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once

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: <20090522200239.GD5354@lenovo>
Date:	Sat, 23 May 2009 00:02:39 +0400
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	Rakib Mullick <rakib.mullick@...il.com>
Cc:	Ingo Molnar <mingo@...e.hu>, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>,
	Yinghai Lu <yhlu.kernel@...il.com>,
	"Maciej W. Rozycki" <macro@...ux-mips.org>
Subject: Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on
	and on.

[Rakib Mullick - Fri, May 22, 2009 at 07:31:29PM +0600]
|   Impact: Reduce text space through efficient coding
| 
|  Determine apic is integrated or not - by calling
| lapic_is_integrated() once from APIC_init_uniprocessor() and keep it
| in a variable integrated_lapic. Thus we can determine apic is
| integrated or not, by checking the variable instead of calling
| lapic_is_integrated() on and on. Marking lapic_is_integrated() as
| __init, which reduces some text space. Also allows us to get away from
| messy #ifdef and inline.
| 
| ---
| Signed-off-by: Rakib Mullick <rakib.mullick@...il.com>
| 
| --- linus/arch/x86/kernel/apic/apic.c	2009-05-21 22:22:15.000000000 +0600
| +++ rakib/arch/x86/kernel/apic/apic.c	2009-05-21 23:27:26.000000000 +0600
| @@ -127,6 +127,9 @@ early_param("nox2apic", setup_nox2apic);
| 
|  unsigned long mp_lapic_addr;
|  int disable_apic;
| +
| +static int integrated_lapic;
| +
|  /* Disable local APIC timer from the kernel commandline or via dmi quirk */
|  static int disable_apic_timer __cpuinitdata;
|  /* Local APIC timer works in C2 */
| @@ -188,13 +191,12 @@ static inline int lapic_get_version(void
|  /*
|   * Check, if the APIC is integrated or a separate chip
|   */
| -static inline int lapic_is_integrated(void)
| +void __init lapic_is_integrated(void)
|  {
| -#ifdef CONFIG_X86_64
| -	return 1;
| -#else
| -	return APIC_INTEGRATED(lapic_get_version());
| -#endif
| +	if (APIC_INTEGRATED(lapic_get_version()))
| +		integrated_lapic = 1;
| +	else
| +		integrated_lapic = 0;
|  }

Hi Rakib,

actually this change could be dangerous. I don't
remember if I saw mixed configuration at all but
I would not be that sure that we will never met it.

Peter? Ingo? Yinghai? Maciej?

| 
|  /*
| @@ -258,7 +260,7 @@ void __cpuinit enable_NMI_through_LVT0(v
|  	v = APIC_DM_NMI;
| 
|  	/* Level triggered for 82489DX (32bit mode) */
| -	if (!lapic_is_integrated())
| +	if (!integrated_lapic)
|  		v |= APIC_LVT_LEVEL_TRIGGER;
| 
|  	apic_write(APIC_LVT0, v);
| @@ -313,7 +315,7 @@ static void __setup_APIC_LVTT(unsigned i
|  	lvtt_value = LOCAL_TIMER_VECTOR;
|  	if (!oneshot)
|  		lvtt_value |= APIC_LVT_TIMER_PERIODIC;
| -	if (!lapic_is_integrated())
| +	if (!integrated_lapic)
|  		lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
| 
|  	if (!irqen)
| @@ -869,7 +871,7 @@ void clear_local_APIC(void)
|  		apic_write(APIC_LVTPC, APIC_LVT_MASKED);
| 
|  	/* Integrated APIC (!82489DX) ? */
| -	if (lapic_is_integrated()) {
| +	if (integrated_lapic) {
|  		if (maxlvt > 3)
|  			/* Clear ESR due to Pentium errata 3AP and 11AP */
|  			apic_write(APIC_ESR, 0);
| @@ -1064,7 +1066,7 @@ void __init init_bsp_APIC(void)
|  	 */
|  	apic_write(APIC_LVT0, APIC_DM_EXTINT);
|  	value = APIC_DM_NMI;
| -	if (!lapic_is_integrated())		/* 82489DX */
| +	if (!integrated_lapic)		/* 82489DX */
|  		value |= APIC_LVT_LEVEL_TRIGGER;
|  	apic_write(APIC_LVT1, value);
|  }
| @@ -1073,7 +1075,7 @@ static void __cpuinit lapic_setup_esr(vo
|  {
|  	unsigned int oldvalue, value, maxlvt;
| 
| -	if (!lapic_is_integrated()) {
| +	if (!integrated_lapic) {
|  		pr_info("No ESR for 82489DX.\n");
|  		return;
|  	}
| @@ -1126,7 +1128,7 @@ void __cpuinit setup_local_APIC(void)
| 
|  #ifdef CONFIG_X86_32
|  	/* Pound the ESR really hard over the head with a big hammer - mbligh */
| -	if (lapic_is_integrated() && apic->disable_esr) {
| +	if (integrated_lapic && apic->disable_esr) {
|  		apic_write(APIC_ESR, 0);
|  		apic_write(APIC_ESR, 0);
|  		apic_write(APIC_ESR, 0);
| @@ -1251,7 +1253,7 @@ void __cpuinit setup_local_APIC(void)
|  		value = APIC_DM_NMI;
|  	else
|  		value = APIC_DM_NMI | APIC_LVT_MASKED;
| -	if (!lapic_is_integrated())		/* 82489DX */
| +	if (!integrated_lapic)		/* 82489DX */
|  		value |= APIC_LVT_LEVEL_TRIGGER;
|  	apic_write(APIC_LVT1, value);
| 
| @@ -1585,6 +1587,8 @@ int __init APIC_init_uniprocessor(void)
|  		pr_info("Apic disabled by BIOS\n");
|  		return -1;
|  	}
| +	/* Since CONFIG_X86_64, so it's integrated.	*/
| +	integrated_lapic = 1;
|  #else
|  	if (!smp_found_config && !cpu_has_apic)
|  		return -1;
| @@ -1599,6 +1603,8 @@ int __init APIC_init_uniprocessor(void)
|  		clear_cpu_cap(&boot_cpu_data, X86_FEATURE_APIC);
|  		return -1;
|  	}
| +	/* Determine APIC is integrated or not	*/
| +	lapic_is_integrated();
|  #endif
| 
|  	enable_IR_x2apic();
| 
	-- Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/