linux-kernel - Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_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, 24 Aug 2012 14:22:26 +0200
From:	Joerg Roedel <joerg.roedel@....com>
To:	Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
CC:	<x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<joro@...tes.org>, Suresh Siddha <suresh.b.siddha@...el.com>,
	Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

On Thu, Aug 23, 2012 at 11:11:40PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:49PM +0200, Joerg Roedel wrote:
> What I miss here is the fact that you extract irq-mapping specific bits from
> disable_IO_APIC() and put them in a separate function named
> irq_remapping_disable_io_apic(). And having a big if in this function probably
> didn't look that sexy so you decided to use function op. Nice move, no
> question but not abvious from the description here and it took a while to figure
> it out based on the code. Well, maybe it is lateâ?¦

Well, yeah, the confusing thing with this patch is, that it does not only
introduce the x86_io_apic_ops.disable call-back but also adds the
infrastructure to change this op (and others). So the infrastructure
change should probably be in a seperate patch. But on the other side the
infrastructure change is small and the patch-set is already really
large.
Is it worth it to split that out?

> I don't want sound to picky here but the term 'modify' is bad I think. Would
> 'overwrite' fit better here? Is 'modify' used someplace else in x86 so you
> follow a common pattern here? Maybe it is just me.

Probably it's just me, but overwrite sounds like 'replace' (== overwrite
all ops with own values). But the code changes only a few of the ops
which need different behavior for interrupt remapping. I think 'modify'
describes that better, no?

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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