linux-kernel - Re: [patch 8/8] x86: apic

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]

Date:	Fri, 15 Aug 2008 13:15:18 +0100 (BST)
From:	"Maciej W. Rozycki" <macro@...ux-mips.org>
To:	Ingo Molnar <mingo@...e.hu>
cc:	Cyrill Gorcunov <gorcunov@...il.com>, tglx@...utronix.de,
	hpa@...or.com, yhlu.kernel@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [patch 8/8] x86: apic - unify init_bsp_APIC

On Fri, 15 Aug 2008, Ingo Molnar wrote:

> > >> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void)
> > >>        */
> > >>       apic_write(APIC_LVT0, APIC_DM_EXTINT);
> > >>       value = APIC_DM_NMI;
> > >> -     if (!lapic_is_integrated())             /* 82489DX */
> > >> +     /* discrete on 82489DX */
> > >> +     if (!lapic_is_integrated())
> > >>               value |= APIC_LVT_LEVEL_TRIGGER;
> > >>       apic_write(APIC_LVT1, value);
> > >>  }
> > >
> > >  Please elaborate.
> > >
> > >  Maciej
> > >
> > 
> > Hi Maciej,
> > 
> > don't really understand what do you mean. [...]
> 
> i suspect the question might have been: 'why this change'.

 Indeed -- the change just moves a comment around from the obvious place 
(where it means: "the condition true applies to the 82489DX") to elsewhere 
while rephrasing it in a way that makes me wonder: "What the hell is that 
meant to mean?"  Perhaps it is clearer to the others, but for me it is 
just obfuscation.

 I think the comment as it is is clear enough and is also clearly visible
when browsing through the source, when you want to spot all the odd bits
related to the discrete APIC.  This is no longer true after the change.  
And any more complete explanation belongs to the definition of
lapic_is_integrated().

> If that was the question, the answer would be: to unify apic_32.c and 
> apic_64.c we first use tiny little changes to bring the two files in 
> sync. Presumably, each such change is a NOP or at least very safe - and 
> clearly bisectable in the worst-case.

 If the obfuscation comes from apic_64.c, then the clean-up should be done
in th other direction IMO.  Not everything that comes from the 64-bit
variation is better than its 32-bit counterpart.

 Please note I do not question the semantic changes contained within this
patch, so the issue of bisectability or code bloat (I am assuming
lapic_is_integrated() expands to 0 on 64-bit; if not, this is clearly
asking for improvement) is irrelevant.

  Maciej
--
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/