linux-kernel - Re: [PATCH 1/2] x86, ioapic: Print out irte with right ioapic index

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:	Mon, 25 Jul 2011 09:28:34 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Naga Chumbalkar <nagananda.chumbalkar@...com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Suresh Siddha <suresh.b.siddha@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] x86, ioapic: Print out irte  with right ioapic index


* Yinghai Lu <yinghai@...nel.org> wrote:

> 
> While checking irte dump in dmesg, the print out is confused ioapic index
> and real io apic id.
> 
> IOAPIC[0]: Set routing entry (1-1 -> 0x31 -> IRQ 1 Mode:0 Active:0 Dest:1)
> IOAPIC[1]: Set IRTE entry (P:1 FPD:0 Dst_Mode:1 Redir_hint:1 Trig_Mode:0 Dlvry_Mode:1 Avail:0 Vector:31 Dest:00000001 SID:00FF SQ:0 SVT:1)
> IOAPIC[0]: Set routing entry (1-2 -> 0x30 -> IRQ 0 Mode:0 Active:0 Dest:1)
> IOAPIC[1]: Set IRTE entry (P:1 FPD:0 Dst_Mode:1 Redir_hint:1 Trig_Mode:0 Dlvry_Mode:1 Avail:0 Vector:30 Dest:00000001 SID:00FF SQ:0 SVT:1)
> 
> The system first ioapic id is 1.
> 
> Try to fix it with passing ioapic idx instead of phys apic id.
> 
> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
> 
> ---
>  arch/x86/kernel/apic/io_apic.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
> +++ linux-2.6/arch/x86/kernel/apic/io_apic.c
> @@ -1255,7 +1255,7 @@ static void ioapic_register_intr(unsigne
>  				      fasteoi ? "fasteoi" : "edge");
>  }
>  
> -static int setup_ioapic_entry(int apic_id, int irq,
> +static int setup_ioapic_entry(int ioapic, int irq,
>  			      struct IO_APIC_route_entry *entry,
>  			      unsigned int destination, int trigger,
>  			      int polarity, int vector, int pin)
> @@ -1266,6 +1266,7 @@ static int setup_ioapic_entry(int apic_i
>  	memset(entry,0,sizeof(*entry));
>  
>  	if (intr_remapping_enabled) {
> +		int apic_id = mpc_ioapic_id(ioapic);
>  		struct intel_iommu *iommu = map_ioapic_to_ir(apic_id);
>  		struct irte irte;
>  		struct IR_IO_APIC_route_entry *ir_entry =

Patch looks good to me in principle, but please clean up this code 
before we put more effort into it.

Firstly, your patch introduces an uncleanliness. I'm not telling you 
what it is because you made that mistake so many times in the past 
that i must assume that you are using me as a coding style compiler 
in essence - that's not very efficient for me, please avoid repeat 
mistakes and improve your patches.

Secondly, the code it modifies has a couple of easily visible 
uncleanlinesses and structure problems which should be fixed first in 
a separate patch, before the printout fix is applied. The function it 
modifies is too large (and has a few style errors in it), etc. etc.

Thanks,

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