linux-kernel - Re: [PATCH 1/1] X86: Probe for PIC and set legacy

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, 11 Apr 2014 15:35:43 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	"K. Y. Srinivasan" <kys@...rosoft.com>, x86@...nel.org,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	devel@...uxdriverproject.org, olaf@...fle.de, apw@...onical.com,
	jasowang@...hat.com, tglx@...utronix.de, JBeulich@...e.com,
	bp@...en8.de
CC:	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

On 04/11/2014 04:02 PM, K. Y. Srinivasan wrote:
>  
> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
> index 2e977b5..0a57a19 100644
> --- a/arch/x86/kernel/i8259.c
> +++ b/arch/x86/kernel/i8259.c
> @@ -299,11 +299,22 @@ static void unmask_8259A(void)
>  static void init_8259A(int auto_eoi)
>  {
>  	unsigned long flags;
> +	unsigned char probe_val = 0xfb;
>  
>  	i8259A_auto_eoi = auto_eoi;
>  
>  	raw_spin_lock_irqsave(&i8259A_lock, flags);
>  
> +	/* Check to see if we have a PIC */
> +	outb(probe_val, PIC_MASTER_IMR);
> +	probe_val = inb(PIC_MASTER_IMR);
> +	if (probe_val == 0xff) {
> +		printk(KERN_INFO "Using NULL legacy PIC\n");
> +		legacy_pic = &null_legacy_pic;
> +		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> +		return;
> +	}
> +
>  	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
>  	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
>  

I think it is important to document what this actually means here.  0xfb
is "mask all except for the cascade."

Arguably, we should do this after masking the slave PIC.

On a whole other subject... the whole __byte() logic is a just plain
horrific piece of crap.  Clearly whomever wrote it had never heard of a
union.

I'm still horrifically confused, though, why we mask IRQ 2 (the cascade
interrupt) in i8259.c, but we explicitly do *not* mask IRQ 2 in
boot/pm.c (nor in the assembly code on which boot/pm.c was based.)  In
fact, it looks like we never unmask it.  I'm guessing that the masking
status of the cascade input simply doesn't matter.

At the very least, though, we shouldn't have 0xfb as a magic number, but
use ~(1U << PIC_CASCADE_IR).

	-hpa

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